Re-derive app config after aioConfigLoader.reload() to fix stale ow credentials in deploy#907
Re-derive app config after aioConfigLoader.reload() to fix stale ow credentials in deploy#907
Conversation
…eload() Three edge cases identified: 1. config.ow.auth passed to deployActions is stale when pre-deploy hook modifies .env 2. this.appConfig cache not cleared after reload 3. Multi-extension deploys all use stale configs from initial load Agent-Logs-Url: https://github.com/adobe/aio-cli-plugin-app/sessions/96f9b49e-4a86-4996-959a-b15807c820b4 Co-authored-by: purplecabbage <46134+purplecabbage@users.noreply.github.com>
After aioConfigLoader.reload(), clear the appConfig cache and re-load the extension config so that env changes from hooks (e.g. new credentials written to .env) are reflected in all config values like ow.auth and ow.namespace, not just process.env used by input resolution. Agent-Logs-Url: https://github.com/adobe/aio-cli-plugin-app/sessions/96f9b49e-4a86-4996-959a-b15807c820b4 Co-authored-by: purplecabbage <46134+purplecabbage@users.noreply.github.com>
Agent-Logs-Url: https://github.com/adobe/aio-cli-plugin-app/sessions/96f9b49e-4a86-4996-959a-b15807c820b4 Co-authored-by: purplecabbage <46134+purplecabbage@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| 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 |
There was a problem hiding this comment.
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.
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.
that's confusing, it shouldn't show up as a change here if this PR was re-based
There was a problem hiding this comment.
🤖 PR Reviewer
The diff adds a config refresh mechanism after pre-deploy hooks run, ensuring updated env vars (e.g., new credentials) are picked up before deploying runtime actions. The implementation is straightforward and the tests provide good coverage for single-extension, cache-clearing, and multi-extension scenarios. One concern is that the refresh only happens when actions are being deployed (inside the if (hasBackend && !flags['skip-actions']) block), and silently falls back to the potentially stale config if refreshedConfigs[name] is missing, which could mask misconfiguration issues.
📝 2 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| // 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 |
There was a problem hiding this comment.
If refreshedConfigs[name] is falsy after reload, the code silently proceeds with the stale config. This could mask a real issue where the extension is no longer present in the refreshed config. At minimum, a warning log should be emitted, or the missing extension should be treated as an error depending on the intended contract.
| this.appConfig = null | |
| if (refreshedConfigs[name]) { | |
| config = { ...setRuntimeApiHostAndAuthHandler(refreshedConfigs[name]) } | |
| } else { | |
| this.log(`Warning: extension '${name}' not found in refreshed config after hook reload, proceeding with pre-hook config`) | |
| } |
| // 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 { |
There was a problem hiding this comment.
The test asserts command.appConfig is null after run() completes. However, this.appConfig = null is set mid-loop before re-deriving, and subsequent code or post-deploy logic within run() might reset it. This test may be fragile and tightly coupled to internal implementation state rather than observable behavior. Consider testing the observable effect (e.g., that getAppExtConfigs was called twice) instead of inspecting internal cache state.
| } finally { | |
| // Verify getAppExtConfigs was called twice: once for initial load, once for post-hook refresh | |
| expect(command.getAppExtConfigs).toHaveBeenCalledTimes(2) |
PR #906 added
aioConfigLoader.reload()beforedeployActionsto pick up.envchanges from hooks. This refreshesprocess.env(sufficient for YAML input resolution viaprocessInputs), but theconfigobject passed todeployActionsis still the one computed at initial load —config.ow.auth,config.ow.namespace, etc. remain stale.Problem
This also affects multi-extension deploys: all extension configs are computed once before the loop, so every extension uses pre-hook values.
Fix
After
aioConfigLoader.reload(), clear the cachedthis.appConfigand re-derive the extension config:Tests added
deployActionsreflects freshow.authwhen pre-deploy hook modifies.envthis.appConfigcache is cleared after reloadAll three tests fail without the fix and pass with it. Full suite (834 tests, 56 suites) passes.