-
Notifications
You must be signed in to change notification settings - Fork 8
[HotFix] Ensure cryp4gh keys are loaded on startup. #1663
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
base: main
Are you sure you want to change the base?
Conversation
5f9be95
to
507e90c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except for the comment about the error message handling. In addition, the deployment of helm chart (ingest and verify services) is failing because of missing c4gh keys.
507e90c
to
24eae14
Compare
I know, it is in the PR description.. |
24eae14
to
69f7594
Compare
func (ts *TestSuite) TestDetectMisingC4GHKeys() { | ||
viper.Set("c4gh.privateKeys", "") | ||
privateKeys, err := config.GetC4GHprivateKeys() | ||
assert.NoError(ts.T(), err) | ||
assert.Equal(ts.T(), 0, len(privateKeys)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this part check? Does it check that config.GetC4GHprivateKeys
translates an empty string to an empty array?
I guess to ensure that the fix won't break the code?
Related issue(s) and PR(s)
This PR closes #1597
This PR closes #1662.
Description
This PR fixes a bug in the code where
Ingest
andVerify
would happily start even when no crypt4gh keys have been configured.How to test
Remove the
c4gh.privateKeys
block from the config file and runmake sda-s3-up
Extra info
Chart tests will fail since the charts are not updated to handle multiple keys yet. (WIP to be completed once this is merged.)