@W-21200034: Add scaffolding to override some config entries at runtime #216
Conversation
anyoung-tableau
changed the title
anyoung-tableau
changed the title
anyoung-tableau
changed the title
| private async getAllowedProjectIds({ | ||
| restApiArgs, | ||
| }: { | ||
| restApiArgs: RestApiArgs; | ||
| }): Promise<Set<string> | null> { | ||
| return ( | ||
| this._testOverrides.projectIds ?? | ||
| ( | ||
| await getConfigWithOverrides({ | ||
| restApiArgs, | ||
| }) | ||
| ).boundedContext.projectIds | ||
| ); | ||
| } |
There was a problem hiding this comment.
None of the scoping logic has changed. The changes in this file are so we retrieve the MCP site settings to see if a tool scoping override exists. Remember, the response from the REST API is cached for the site for n=10 minutes so once we call it once, all subsequence responses come from cache for the next 10 minutes.
| it('should set mcpSiteSettingsCheckIntervalInMinutes to default when not specified', () => { | ||
| const config = new Config(); | ||
| expect(config.mcpSiteSettingsCheckIntervalInMinutes).toBe(10); | ||
| }); |
There was a problem hiding this comment.
Diff is kinda bad in this file. I moved the tests for overridable config to their own file and added tests for MCP_SITE_SETTINGS_CHECK_INTERVAL_IN_MINUTES and ENABLE_MCP_SITE_SETTINGS.
| includeTools: Array<ToolName>; | ||
| excludeTools: Array<ToolName>; |
There was a problem hiding this comment.
I had initially left all overridable config items in the Config class, leaving the responsibility of checking the site settings for an updated value on the developer. I felt this would be prone to bugs so I moved all the overridable items to OverridableConfig class instead.
| import { exportedForTesting } from './overridableConfig.js'; | ||
| import { stubDefaultEnvVars } from './testShared.js'; | ||
|
|
||
| describe('OverridableConfig', () => { |
There was a problem hiding this comment.
All these tests are copy/paste from config.test.ts
| function filterEnvVarsToOverridable( | ||
| environmentVariables: Record<string, string | undefined>, | ||
| ): Record<OverridableVariable, string | undefined> { | ||
| return Object.fromEntries( | ||
| Object.entries(environmentVariables).filter(([key]) => isOverridableVariable(key)), | ||
| ) as Record<OverridableVariable, string | undefined>; | ||
| } |
There was a problem hiding this comment.
This is the magic that ensures only the environment variables that we have specified as overridable get overridden.
| | 'tableau:views:download' | ||
| | 'tableau:insight_brief:create'; | ||
| | 'tableau:insight_brief:create' | ||
| | 'tableau:mcp_site_settings:read'; |
There was a problem hiding this comment.
This is currently a "fake" scope but will be the one we use when adding the new Get MCP Site Settings REST API
| export const mcpSiteSettingsSchema = z.record(z.string(), z.string()); | ||
| export type McpSiteSettings = z.infer<typeof mcpSiteSettingsSchema>; |
There was a problem hiding this comment.
The idea is that the "Get MCP Site Settings" REST API will return a JSON object in this format, representing key-value pairs of environment variable names and their values
{
"EXCLUDE_TOOLS": "query-datasource",
"INCLUDE_DATASOURCE_IDS": "2d935df8-fe7e-4fd8-bb14-35eb4ba31d4"
}
| } & ( | ||
| | { | ||
| requestId: RequestId; | ||
| disableLogging?: false; | ||
| } | ||
| | { | ||
| disableLogging: true; | ||
| } |
There was a problem hiding this comment.
If it's not clear, the requestId is only used in logging. This type makes it so you can't provide it when disableLogging is true. TypeScript rules and I'll die on this hill. :)
1 participant
These changes lay the groundwork for @W-21200059: Add Get/Update MCP site settings REST APIs.
Once the "Get MCP Site Settings" REST API is available, we can call it to get the actual site settings. Currently, the
ENABLE_MCP_SITE_SETTINGSfeature is disabled but can be enabled for testing. When enabled, the method that retrieves the MCP site settings returns a placeholder empty object (implying no overrides).