fix: add support for Tableau multi-site deployment #463
Conversation
shuichiro-makigaki
requested review from
allisonsuarez,
dikshathakur3119,
feng-tao and
jinhyukchang
as code owners
Tableau supports multi-tenancy as "site", but current databuilder implementation does not support the feature. Now, dashboard_url and dashboard_group_url contain site name (e.g. https://tableau-server/#/site/my-site/...). Signed-off-by: Shuichiro MAKIGAKI <shuichiro.makigaki@gmail.com>
shuichiro-makigaki
force-pushed
the
tableau-multi-site-support
branch
from
ca92311
to
dddf7a1
Compare
|
cc @alevene if you have time to take a look |
There was a problem hiding this comment.
@shuichiro-makigaki your current changes look almost entirely good to me, just one small consistency change & and a non-blocking suggestion on providing a default site name. As far as I can tell, I don't think either vizportalUrlId is used anywhere else, so I think we're good to go there.
| @@ -39,6 +40,10 @@ def execute(self) -> Iterator[Dict[str, Any]]: | |||
| if workbook['projectName'] not in | |||
| self._conf.get_list(TableauGraphQLApiMetadataExtractor.EXCLUDED_PROJECTS, [])] | |||
| base_url = self._conf.get(TableauGraphQLApiMetadataExtractor.TABLEAU_BASE_URL) | |||
| site_name = self._conf.get_string(TableauGraphQLApiMetadataExtractor.SITE_NAME) | |||
There was a problem hiding this comment.
I was not able to find a concrete answer for this when searching the Tableau docs, but my understanding based on the code below is that unless specifically configured for multiple sites (tenants), the /site/{site_name} portion of the URL is not present; that is, for a single-tenant installation, the URLs will still have the form https://tableau-server/my-site/ so that there is no specific site given and the site_name would be ''. Additionally, the only other place SITE_NAME is used is here during the authentication process, which also expects an empty string for a single-site installation.
Because of this, I think it would be wise to provide a default value of '' for this call to conf.get_string in both of these instances, since a user would not necessarily need to specify a SITE_NAME if they only have one site, and failing to specify a value they don't strictly need to provide shouldn't cause a failure. On the other hand, I could also see a case for not providing a default, as one could argue that one does need to specify the config value since it is actually "used", but it just doesn't have any characters inside it.
Since this potential problem already exists elsewhere in the code outside the scope of this PR, and could be argued against, I wouldn't consider this a blocking issue, but just a proposal. I'm curious to get others thoughts on this issue as well.
There was a problem hiding this comment.
my understanding based on the code below is that unless specifically configured for multiple sites (tenants), ...
You are correct. Ref: https://help.tableau.com/current/pro/desktop/en-us/embed_structure.htm
I think it would be wise to provide a default value of '' for this call to conf.get_string in both of these instances, since a user would not necessarily need to specify a SITE_NAME if they only have one site, and failing to specify a value they don't strictly need to provide shouldn't cause a failure.
Sounds good. I added a default value for a site name.
Speaking only of the tableau_dashboard_extractor module, other values such as base_url are necessary to work. (cluster name also looks necessary for making dashboard key in a database.) As you said, this is a potential problem, but I'd like others to go out of my PR scope.
| site_url_path = "" | ||
| if site_name != "": |
There was a problem hiding this comment.
Please use single quotes ('') here for consistency.
Default site name is '', and site name conf is optional. Signed-off-by: Shuichiro MAKIGAKI <shuichiro.makigaki@gmail.com>
shuichiro-makigaki
force-pushed
the
tableau-multi-site-support
branch
from
be8c70c
to
c3671d2
Compare
|
@feng-tao this PR looks good to me! |
|
thanks @ccarterlandis for the review and @shuichiro-makigaki for the contribution! |
Summary of Changes
Tableau supports multi-tenancy as a "site", but the current databuilder implementation does not support the feature.
Now, dashboard_url and dashboard_group_url contain site name (e.g. https://tableau-server/#/site/my-site/...).
Tests
Add some test cases to check dashboard_url and dashboard_group_url
Documentation
CheckList
Make sure you have checked all steps below to ensure a timely review.
make test