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?
There was a problem hiding this comment.
This change is necessary because we have the context on which configuration is from remote and which is from local. We need to continue supporting older app versions with scopes in its remote config when linking. The app access extension currently accepts both scopes and required_scopes and the extension doesn't have the context on which config is from remote and which is from local.
1. Remote has required_scopes and local has scopes
We need to delete local scopes and accept required_scopes to be used in the config
2. Remote has scopes and local has required_scopes
We need to delete local required_scopes and accept remote scopes to be used in the config. Further down the link pipeline, we'll detect that scopes is still being used and prompt the user to migrate to required_scopes
3. Remote has scopes and local has scopes
Overwrite local scopes values from remote and prompt user to migrate to required_scopes further down the link pipeline
4. Remote has required_scopes and local has required_scopes
Overwrite local required_scopes values from remote
If we don't delete the required_scopes or scopes here when we know the context of which is from remote, the extension won't know which one is the correct version of the config.
| @@ -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.
There was a problem hiding this comment.
This was my concern as well when I was prototyping. The TOML content didn't change when I modified the module instance's configuration. I had to modify the scopes in 3 places still:
- The app instance's
this.configurationto have the new data (required for re-writing the TOML) - Re-write the TOML to reflect the new changes (this method)
- Each module instance's
extension.configuration
I couldn't find a way to only modify extension.configuration and let that change be reflected in the TOML and app directly. I will reach out to the CLI team to see if I'm missing something when modifying this config
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