env-kvstore: warn when env specific values are missing from kvstore#489
env-kvstore: warn when env specific values are missing from kvstore#489bpdohall wants to merge 4 commits into
Conversation
rosstimothy
left a comment
rosstimothy
left a comment
There was a problem hiding this comment.
Can we add some test coverage to prevent regressions?
| } | ||
| if envArn != "" && envStore.IsEmpty() && err == nil { | ||
| slog.Info("environment-specific values are empty in Secrets Manager, only repo-level values will be available", "environment", s.ghaClaims.Environment, "arn", envArn) | ||
| err = envStoreError{msg: fmt.Sprintf("no environment-specific values found in Secrets Manager for environment %s", s.ghaClaims.Environment)} |
There was a problem hiding this comment.
Will an envStoreError error ever be populated with a different message? Should an envStoreError have an environment instead of a msg, and should this fmt.Sprintf be in (envStoreError) Error() instead of here?
|
Testing ("docker shared-workflows/tools/env-kvstore summary"): missing environment, job is successful with warnings: https://github.com/gravitational/bpdohall-platformops-gha/actions/runs/28175820640 |
Do we execute these jobs against every change to this tool? Can we add unit tests that are required to pass before any change to this tool merges to prevent regressions? |
I executed those jobs manually. Added unit tests that cover secretsmanager API responses and errors in d5383b6 |
bpdohall
force-pushed
the
brendan/env-kvstore-warn-on-missing-env-values
branch
from
1b6c662 to
6517aed
Compare
bpdohall
force-pushed
the
brendan/env-kvstore-warn-on-missing-env-values
branch
from
6517aed to
d5383b6
Compare
4 participants
Currently failing for repositories invoking the action with an environment set but the environment has not been configured in the secrets manager kvstore.
Because we're using ABAC/tag-based access to secrets, there's never going to be a "resource not found" error.
This PR simplifies handling of errors populating the environment-specific values so they are treated and logged as a warning instead of a failure.