Migrate scopes to required_scopes #5389
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
zzooeeyy
force-pushed
the
zl/migrate_scopes_to_required_scopes
branch
from
54472ee to
0b02232
Compare
zzooeeyy
force-pushed
the
zl/migrate_scopes_to_required_scopes
branch
from
0b02232 to
3f89a13
Compare
| @@ -108,6 +108,8 @@ export async function linkedAppContext({ | |||
| await addUidToTomlsIfNecessary(localApp.allExtensions, developerPlatformClient) | |||
| } | |||
|
|
|||
| await localApp.migratePendingSchemaChanges() | |||
There was a problem hiding this comment.
At this level of abstraction there is nothing specific about "schema changes" to be known. What you've built here is a more generic "Extensions can run arbitrary code during link" mechanism.
Some thoughts:
- Since the abstract solution here isn't specific to migrations, we could have a more general name (not sure what it would be).
- Alternatively, I think there is value in constraining usages of this function to actual transformations. In other words: is there a way to design this interface such that implementations are constrained in the things they can do?
Additionally, this solution relies on mutation of the underlying configuration object, and passing in mutable properties can be a source of bugs, so it might be worth looking at a solution that doesn't involve mutating the extension.config object.
I think #2 might be worth pursuing. Today your interface is is (extension: ExtensionInstance) => Promise<void>, which is generic. I would suggest trying an alternative where the interface is something like (config: TConfiguration) => TConfiguration (provide a function that maps a configuration to a new configuration). This would constrain the possible implementations to mutating a config, which is more in line with what you want here.
| await Promise.all([this.realExtensions.map((ext) => ext.migratePendingSchemaChanges())]) | ||
| } | ||
|
|
||
| async migrateScopesToRequiredScopes() { |
There was a problem hiding this comment.
Why is this needed here still? I see you are doing the mutation in the app_config_app_access file, so this seems redundant? I'm probably missing something.
| @@ -378,13 +378,29 @@ async function overwriteLocalConfigFileWithRemoteAppConfiguration(options: { | |||
| remoteAppConfiguration = buildAppConfigurationFromRemoteAppProperties(remoteApp, locallyProvidedScopes) | |||
| } | |||
|
|
|||
| // Create a clean version of the local config without scopes/require_scopes | |||
There was a problem hiding this comment.
link is also a "platform" class so in general it shouldn't have reference to concrete extension implementations like access scopes. Given you have the general interface for mutating the TOML, why is this code also necessary?
| @@ -43,3 +43,24 @@ export async function patchAppConfigurationFile({path, patch, schema}: PatchToml | |||
| export function replaceArrayStrategy(_: unknown[], newArray: unknown[]): unknown[] { | |||
| return newArray | |||
| } | |||
|
|
|||
| export async function replaceScopesWithRequiredScopesInToml(path: string) { | |||
There was a problem hiding this comment.
Same Q here - why is this required? I wouldn't expect this file to include module-specific code.
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist