@W-21817321: Add Tableau MCP site level configuration support#291
@W-21817321: Add Tableau MCP site level configuration support#291stephendeoca wants to merge 15 commits intomainfrom
Conversation
stephendeoca
changed the title
src/utils/mcpSiteSettings.ts
Outdated
| }): Promise<McpSiteSettings | undefined> { | ||
| const { config, tableauAuthInfo } = restApiArgs; | ||
| if (!config.enableMcpSiteSettings) { | ||
| if (!config.enableMcpSiteSettings || RestApi.version < MCP_SITE_SETTINGS_MIN_REST_API_VERSION) { |
There was a problem hiding this comment.
While this may work in practice, it's generally not recommended to compare strings like this in JS since it's not doing numerical comparison, it's doing string comparison.
I suggest adding a versionAsNumber property to RestApi that uses parseFloat() to get the numerical version value.
There was a problem hiding this comment.
I'm on board, but we actually need to split it into parts since we want 3.29 < 3.290 to return true. The 29th version of the REST API should be less than the 290th version. Similar to how version 3.1 came before 3.10.
I'm going to add a versionIsAtLeast method instead
There was a problem hiding this comment.
Oh yeah, good call
src/utils/mcpSiteSettings.ts
Outdated
| // getting mcpSiteSettings fails when the `MCPSettingsEnabled` feature is disabled on a site | ||
| // we should still cache the result to avoid repeated requests | ||
| // TODO: should we log here? there's also an internal server error if the settings are in a bad state and need to be overwritten. maybe log that one? |
There was a problem hiding this comment.
If the feature flag is disabled that means either the TabServ customer disabled it themselves or we disabled it on Cloud in response to some livesite issue. In either case I agree with you: I think we should cache the response.
For internal server errors, I know we've discussed silently swallowing the error and returning an empty set, but we should revisit that. I'm now leaning toward surfacing the error to the user so they can go complain to their admin who specified a busted config.
| ...restApiArgs, | ||
| jwtScopes: ['tableau:mcp_site_settings:read'], | ||
| callback: async (restApi) => | ||
| await restApi.mcpSettingsMethods.getMcpSiteSettings({ siteId: restApi.siteId }), |
There was a problem hiding this comment.
Is the new REST API available in any of the dev/test pods? Let's verify it can be called using the new bearer token.
There was a problem hiding this comment.
The feature flag is disabled for dev/test pods, i'm working with Jill on getting that done bc its different than server.
|
|
||
| type SiteNameOrSiteId = string; | ||
|
|
||
| const MCP_SITE_SETTINGS_MIN_REST_API_VERSION = '3.29'; |
There was a problem hiding this comment.
It might make sense to gate by Tableau version, not REST API version, since we already have the tools to compare Tableau product versions
There was a problem hiding this comment.
I decided to not gate using Tableau version, since we'd have to get productVersion which either gets from serverInfo cache or makes a restApi request for serverInfo. Which feels like a hat on a hat since the restApi static version will already be set if the serverInfo was cached. Also i'm trying to avoid stacking / layering possible restApi calls in overridable config.
2 participants
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Pull Request Template
Description
Fetches and applies MCP Site Settings in
overridableConfig.ts(the single source of truth for overriding variables). Added docs for site settings and updated existing docs to reflect new exemptions / behavior.Motivation and Context
Site and server admins can now configure MCP variables on a per site basis.
Type of Change
How Has This Been Tested?
Unit tests, Live site testing
Related Issues
N/A
Checklist
npm run version. For example,use
npm run version:patchfor a patch version bump.environment variable or changing its default value.
Contributor Agreement
By submitting this pull request, I confirm that:
its Contribution Checklist.