[dagster-tableau] Implement refreshable workbooks #24862
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @maximearmstrong and the rest of your teammates on |
| time.sleep(poll_interval) | ||
| job = self.get_job(job_id=job.id) | ||
|
|
||
| if job.finish_code == -1: |
There was a problem hiding this comment.
might be nice to stick these codes in enum values or constants so it's clearer what they represent
There was a problem hiding this comment.
Done in 1ddbbaa
| dagster_tableau_translator: Type[DagsterTableauTranslator], | ||
| ) -> Sequence[CacheableAssetsDefinition]: | ||
| """Returns a set of CacheableAssetsDefinition which will load Tableau content from | ||
| the workspace and translates it into AssetSpecs, using the provided translator. | ||
|
|
||
| Args: | ||
| start_workbook_refresh_requests (Sequence[StartWorkbookRefreshRequest]): A list of |
There was a problem hiding this comment.
I think it's a bit unclear from the docstring what providing this argument does - this creates an SDA for each workbook associated with a request, right?
In general I think this name is a bit confusing. The user is submitting a "template" for a request and not a request itself (since the requests are made at runtime). Moreover, each request passed here instructs Dagster to build an SDA to allow the workbook to be refreshed. This is not very clearly represented in the name/docstring.
I had similar feedback for the Looker equivalent (RequestStartPdtBuild), which I guess this is modeled after. In that case the justification was that calling it RequestStartPdtBuild matched the name in documentation so it was easier for users to understand the different configuration options and to look up docs. In this case, since the only argument is the workbook ID, I don't think we get the same utility out of retaining the name from Tableau's API.
I'd suggest calling it something like enable_refreshing_workbooks and take a sequence of workbook IDs, unless we expect to add other parameters down the line. Even if we retain the dataclass name, I think the argument or docstring could be updated to be more clear, but curious for your thoughts.
There was a problem hiding this comment.
Wrote this up before seeing Ben's parallel comment:
I know it's built to be parallel to a similar concept in the Looker integration, but the name StartWorkbookRefreshRequest strikes me as a little odd – it's a verb if I'm reading it correctly, but classes are normally nouns. Two thoughts:
- Is there a shorter noun that would make more sense?
- Do we need a class for this at all? E.g. maybe just something like
refreshable_workbook_ids?
There was a problem hiding this comment.
I also think we should consider changing the name in Looker.
There was a problem hiding this comment.
That makes a lot of sense. This is modeled after the Looker integration, but with this feedback I agree that we should make the changes in both integrations.
There was a problem hiding this comment.
Changes made in 3c010c6
| @@ -15,6 +15,11 @@ def _clean_asset_name(name: str) -> str: | |||
| return re.sub(r"[^a-z0-9A-Z.]+", "_", name).lower() | |||
|
|
|||
|
|
|||
| @record | |||
| class StartWorkbookRefreshRequest: | |||
There was a problem hiding this comment.
nit: docstring, since this is user facing
There was a problem hiding this comment.
Class removed in 3c010c6
maximearmstrong
force-pushed
the
maxime/use-tableau-server-client-for-dagster-tableau
branch
from
16eeed9
to
4d5429f
Compare
maximearmstrong
force-pushed
the
maxime/implement-tableau-refreshable-workbooks
branch
from
dca7f72
to
7cefd11
Compare
maximearmstrong
force-pushed
the
maxime/use-tableau-server-client-for-dagster-tableau
branch
from
4d5429f
to
ad95ae9
Compare
maximearmstrong
force-pushed
the
maxime/implement-tableau-refreshable-workbooks
branch
from
7cefd11
to
f764c68
Compare
maximearmstrong
force-pushed
the
maxime/implement-tableau-refreshable-workbooks
branch
2 times, most recently
from
a805ad2
to
3c010c6
Compare
maximearmstrong
changed the title
maximearmstrong
force-pushed
the
maxime/implement-tableau-refreshable-workbooks
branch
from
3c010c6
to
1ddbbaa
Compare
maximearmstrong
force-pushed
the
maxime/implement-tableau-refreshable-workbooks
branch
from
8dfb00b
to
f0bb010
Compare
maximearmstrong
deleted the
maxime/implement-tableau-refreshable-workbooks
branch
Summary & Motivation
Uses https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_ref_workbooks_and_views.htm#update_workbook_now to refresh workbooks built on top of extracts.
There is no API to fetch only the workbooks that can be refreshed with the Update Workbook Now endpoint, so we need to manually specify them.
How I Tested These Changes
BK with updated tests, local.
Changelog
NOCHANGELOG