-
Notifications
You must be signed in to change notification settings - Fork 35
Re-derive app config after aioConfigLoader.reload() to fix stale ow credentials in deploy #907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
229b177
5e31357
ebf015c
b9e8d95
cf881a1
1474809
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -232,6 +232,14 @@ class Deploy extends BuildCommand { | |||||||||||||
| this.error(hookResults.failures.map(f => `${f.plugin.name} : ${f.error.message}`).join('\nError: '), { exit: 1 }) | ||||||||||||||
| } | ||||||||||||||
| aioConfigLoader.reload() | ||||||||||||||
| // Re-derive the app config so that env changes from hooks | ||||||||||||||
| // (e.g. new credentials written to .env) are reflected in all | ||||||||||||||
| // config values, not just process.env used by input resolution. | ||||||||||||||
| this.appConfig = null | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Suggested change
|
||||||||||||||
| const refreshedConfigs = await this.getAppExtConfigs(flags) | ||||||||||||||
| if (refreshedConfigs[name]) { | ||||||||||||||
| config = { ...setRuntimeApiHostAndAuthHandler(refreshedConfigs[name]) } | ||||||||||||||
| } | ||||||||||||||
| deployedRuntimeEntities = await rtLib.deployActions(config, { filterEntities, useForce: flags['force-deploy'] }, onProgress) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -887,6 +887,129 @@ describe('run', () => { | |||||||
| ) | ||||||||
| }) | ||||||||
|
|
||||||||
| test('deploy should pass refreshed config to deployActions when pre-deploy hook changes env', async () => { | ||||||||
| const staleAuth = 'stale-auth-token' | ||||||||
| const freshAuth = 'fresh-auth-from-hook' | ||||||||
|
|
||||||||
| // Initial config loaded before hooks run has stale auth | ||||||||
| const staleAppConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: staleAuth, namespace: 'test-ns' } | ||||||||
| }) | ||||||||
|
|
||||||||
| // After reload, the config should be re-derived with the fresh auth | ||||||||
| const freshAppConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: freshAuth, namespace: 'test-ns' } | ||||||||
| }) | ||||||||
|
|
||||||||
| // First call returns stale config (initial load), second call returns fresh config (after reload) | ||||||||
| command.getAppExtConfigs | ||||||||
| .mockResolvedValueOnce(staleAppConfig) | ||||||||
| .mockResolvedValueOnce(freshAppConfig) | ||||||||
|
|
||||||||
| // Pre-deploy hook simulates writing a new auth token to .env | ||||||||
| helpers.runInProcess | ||||||||
| .mockImplementationOnce(async () => { | ||||||||
| // In real code, a hook might write new credentials to .env | ||||||||
| process.env.AIO_RUNTIME_AUTH = freshAuth | ||||||||
| return undefined // no script found | ||||||||
| }) | ||||||||
| .mockResolvedValueOnce(undefined) // deploy-actions | ||||||||
| .mockResolvedValueOnce(undefined) // post-app-deploy | ||||||||
|
|
||||||||
| command.argv = ['--no-web-assets'] | ||||||||
| try { | ||||||||
| await command.run() | ||||||||
|
|
||||||||
| expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1) | ||||||||
| const deployedConfig = mockRuntimeLib.deployActions.mock.calls[0][0] | ||||||||
| // The config passed to deployActions should reflect the refreshed auth, | ||||||||
| // not the stale value from the initial config load | ||||||||
| expect(deployedConfig.ow.auth).toBe(freshAuth) | ||||||||
| } finally { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test asserts
Suggested change
|
||||||||
| delete process.env.AIO_RUNTIME_AUTH | ||||||||
| } | ||||||||
| }) | ||||||||
|
|
||||||||
| test('deploy should clear appConfig cache after reload so subsequent loads are fresh', async () => { | ||||||||
| const staleAppConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: 'stale-auth', namespace: 'stale-ns' } | ||||||||
| }) | ||||||||
| const freshAppConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: 'fresh-auth', namespace: 'fresh-ns' } | ||||||||
| }) | ||||||||
|
|
||||||||
| command.getAppExtConfigs | ||||||||
| .mockResolvedValueOnce(staleAppConfig) | ||||||||
| .mockResolvedValueOnce(freshAppConfig) | ||||||||
|
|
||||||||
| helpers.runInProcess | ||||||||
| .mockResolvedValueOnce(undefined) // pre-app-deploy | ||||||||
| .mockResolvedValueOnce(undefined) // deploy-actions | ||||||||
| .mockResolvedValueOnce(undefined) // post-app-deploy | ||||||||
|
|
||||||||
| command.argv = ['--no-web-assets'] | ||||||||
| await command.run() | ||||||||
|
|
||||||||
| // After deploy with reload, the appConfig cache should be cleared | ||||||||
| // so that a subsequent getAppExtConfigs call returns fresh config. | ||||||||
| // This matters for multi-extension loops and post-deploy logic. | ||||||||
| expect(command.appConfig).toBeNull() | ||||||||
| }) | ||||||||
|
|
||||||||
| test('deploy should pass refreshed config for each extension in multi-extension deploy', async () => { | ||||||||
| const staleAuth = 'stale-auth-multi' | ||||||||
| const freshAuth = 'fresh-auth-multi' | ||||||||
|
|
||||||||
| const staleMultiConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: staleAuth, namespace: 'test-ns' } | ||||||||
| }, 'app-exc-nui') | ||||||||
|
|
||||||||
| // Fresh config returned after each reload | ||||||||
| const freshMultiConfig = createAppConfig({ | ||||||||
| ...command.appConfig, | ||||||||
| runtime: { auth: freshAuth, namespace: 'test-ns' } | ||||||||
| }, 'app-exc-nui') | ||||||||
|
|
||||||||
| // Initial load returns stale config, then each per-extension reload returns fresh config | ||||||||
| command.getAppExtConfigs | ||||||||
| .mockResolvedValueOnce(staleMultiConfig) // initial load in run() | ||||||||
| .mockResolvedValue(freshMultiConfig) // all subsequent reload calls | ||||||||
|
|
||||||||
| // Each extension: pre-app-deploy, deploy-actions, post-app-deploy | ||||||||
| // app-exc-nui has 3 extensions (application, dx/asset-compute/worker/1, dx/excshell/1) | ||||||||
| // First extension's pre-deploy hook changes env | ||||||||
| helpers.runInProcess | ||||||||
| .mockImplementationOnce(async () => { | ||||||||
| // First extension's pre-deploy hook modifies .env | ||||||||
| process.env.AIO_RUNTIME_AUTH = freshAuth | ||||||||
| return undefined | ||||||||
| }) | ||||||||
| .mockResolvedValue(undefined) // all other hook calls return undefined | ||||||||
|
|
||||||||
| mockExtRegExcShellAndNuiPayload() | ||||||||
| command.argv = ['--no-web-assets'] | ||||||||
| try { | ||||||||
| await command.run() | ||||||||
|
|
||||||||
| // All 3 extensions should have been deployed | ||||||||
| expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(3) | ||||||||
|
|
||||||||
| // The second and third extension deploys should use the fresh auth, | ||||||||
| // not the stale auth from the initial config load | ||||||||
| const secondExtConfig = mockRuntimeLib.deployActions.mock.calls[1][0] | ||||||||
| const thirdExtConfig = mockRuntimeLib.deployActions.mock.calls[2][0] | ||||||||
| expect(secondExtConfig.ow.auth).toBe(freshAuth) | ||||||||
| expect(thirdExtConfig.ow.auth).toBe(freshAuth) | ||||||||
| } finally { | ||||||||
| delete process.env.AIO_RUNTIME_AUTH | ||||||||
| } | ||||||||
| }) | ||||||||
|
|
||||||||
| test('deploy (has deploy-actions and deploy-static hooks)', async () => { | ||||||||
| command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig)) | ||||||||
| const noScriptFound = undefined | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reloading of config (nulling out of this.config) should be a function in BaseCommand.js that is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is from the last pr that was already approved and merged.
https://github.com/adobe/aio-cli-plugin-app/pull/906/changes#diff-ef6fdb9ce6a580102fb8f0ca000a2352ba0ad42dee61d6951457984ec7499b61R234
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's confusing, it shouldn't show up as a change here if this PR was re-based