Skip to content
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

Ensure we check for the key to exist before attempting to read it #166

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

sarahhenkens
Copy link
Contributor

This PR fixed #165, ensuring we check for the key existence before we pass it into json.Marshal.

data[secretKey] returns nil and json.Marshal(nil) hides the fact that the secretKey doesn't exist.

➜ make test 
gotestsum --format=short-verbose 
PASS internal/version.TestGetVersion (0.00s)
PASS internal/version
PASS internal/config.TestParseParametersFromYaml (0.00s)
PASS internal/config.TestParseParameters (0.00s)
PASS internal/config.TestParseConfig (0.00s)
PASS internal/config.TestParseConfig_Errors (0.00s)
PASS internal/config.TestValidateConfig (0.00s)
PASS internal/config
PASS TestListen (0.00s)
PASS .
PASS internal/provider.TestEnsureV1Prefix (0.00s)
PASS internal/provider.TestGenerateRequest (0.00s)
PASS internal/provider.TestKeyFromData (0.00s)
PASS internal/provider.TestKeyFromDataMissingKey (0.00s)
PASS internal/provider.TestHandleMountRequest (0.12s)
PASS internal/provider
PASS internal/client.TestNew (1.81s)
PASS internal/client.TestConfigPrecedence (0.00s)
PASS internal/client.TestNew_Error (0.00s)
PASS internal/client
EMPTY internal/server

DONE 15 tests in 6.840s

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for including so much detail and tests upfront! I just have one suggestion to include more detail in the error message, and please could you also add a short entry to the changelog? Then I think this will be ready to merge.

internal/provider/provider.go Show resolved Hide resolved
@sarahhenkens sarahhenkens requested a review from tomhjp July 20, 2022 15:53
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope you don't mind I made 3 characters worth of nit edits just to save another round of review :)

Thanks for contributing this! It looks great 👌

internal/provider/provider.go Show resolved Hide resolved
@tomhjp tomhjp merged commit 3a04423 into hashicorp:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provider writes null if key does not exist at the secret path
2 participants