Fix small issues preventing sunlight-secretmanager from working#12
Merged
Conversation
Drop the exhaustruct linter. The AWS SDK makes extensive use of zero value fields in structs, so this linter isn't a good match for this program.
mcpherrinm
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes several bugs in the sunlight-secretmanager tool to make it functional. The changes address configuration field deprecation, error handling for missing secrets, and correct API usage for updating existing secrets.
- Replace deprecated
Namefield withShortNamein configuration and logging - Add proper error handling for secrets that don't exist (ResourceNotFoundException)
- Switch from
CreateSecrettoPutSecretValueAPI for updating existing secrets
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/sunlight-secretmanager/main.go | Updates logging to use ShortName instead of deprecated Name field |
| cmd/sunlight-secretmanager/config.go | Changes configuration structure to use ShortName field |
| cmd/sunlight-secretmanager/config_test.go | Updates test data to match new ShortName field |
| cmd/sunlight-secretmanager/seed.go | Adds error handling for missing secrets and switches to PutSecretValue API |
| cmd/sunlight-secretmanager/seed_test.go | Updates mock implementation to use PutSecretValue instead of CreateSecret |
| .golangci.yaml | Disables exhaustruct linter to allow partial struct initialization |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mcpherrinm
force-pushed
the
mattm-fixups
branch
from
c6dc610 to
d7ac1f1
Compare
aarongable
approved these changes
Aug 11, 2025
Contributor
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
LGTM. My primary worry is that PutSecretValue is not idempotent -- it's happy to create a new version of a secret even when one already exists. It's unclear to me how best to protect against this. But afaict this problem exists with CreateSecret too, so it's not a new issue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a few bugs in sunlight-secretsmanager found when trying to use it:
The "Name" value in config was deprecated and isn't in our current Sunlight configs - use shortname for logging instead
Trying to get a secret with no current version returns an error, which must be handled.
The code incorrectly used CreateSecret, but the secret already exists - we need to put content instead.