TOPS-2701 - construct cloud lookup clients lazily in output resolution - #21
TOPS-2701 - construct cloud lookup clients lazily in output resolution#21JSimmsDev wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the variable resolution process by lazily initializing AWS and GCP secret managers only when a remote lookup is required, which avoids credential errors when they are not needed. The review feedback recommends initializing the manager variables (ssm_store, cf_exports, and gcp_secret_manager) to None prior to the conditional checks to prevent potential static analysis warnings or runtime UnboundLocalError issues.
There was a problem hiding this comment.
🟡 Changes recommended
The AWS lazy-construction logic still eagerly constructs both AWS lookup clients together when any one AWS lookup prefix is present, which can unnecessarily trigger unused client initialization and contradicts the stated behavior of constructing clients only for matching prefixes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the envars output resolution pipeline to avoid eagerly constructing cloud remote-lookup clients (AWS SSM/CloudFormation, GCP Secret Manager) when the resolved values don’t actually use remote-lookup prefixes, preventing credential/auth failures on hosts that lack those cloud credentials.
Changes:
- Adds prefix scanning in
_get_resolved_variablesso remote-lookup clients are only constructed when lookup-prefixed values are present. - Adds regression tests ensuring all-plaintext AWS-keyed and GCP-keyed files can be resolved without cloud credentials by asserting the relevant client constructors are not invoked.
File summaries
| File | Description |
|---|---|
| src/envars/main.py | Makes remote-lookup client construction conditional based on presence of lookup prefixes during output resolution. |
| tests/test_cli.py | Adds regression tests for plaintext AWS/GCP keyed files to ensure output doesn’t require cloud credentials. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e of its own prefix - Replace the any() pre-scan with None-initialized clients built at first use, so a file using only parameter_store: no longer constructs the CloudFormationExports client (and vice versa) - Covers both review asks: per-prefix construction and None-initialized variables - New regression test: parameter_store lookup does not construct the CloudFormationExports client
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the stated root cause, and is covered by targeted regression tests for the failure mode and lazy-construction behavior.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
JIRA Ticket: TOPS-2701
Summary
envars outputrequiring cloud credentials for files with no remote-lookup values: the resolve path constructedGCPSecretManager()unconditionally for any GCP-keyed file (andSSMParameterStore()/CloudFormationExports()for AWS-keyed), andSecretManagerServiceClient()eagerly runsgoogle.auth.default()— so an all-plaintext GCP-keyed file dies withDefaultCredentialsErroron any host without GCP ADC.gcp_secret_manager:/parameter_store:/cloudformation_export:).Root cause
_get_resolved_variablesbuilt the provider's lookup clients before scanning values for lookup prefixes; the GCP client authenticates at construction time. Surfaced by timeoutdigital/premium#462 (first all-plaintext GCP-keyed file meant to be resolved on an AWS CodeBuild host).Test plan
assert_not_called)envars -f envars-gcp.yml output --env prod --loc awswith stripped ADC failed withDefaultCredentialsErrorbefore, resolves cleanly afterRollback
git revert— behaviour-preserving for files that do use remote lookups (clients still constructed on demand).