refactor keyring state store methods #23943
Merged
Conversation
This file contains 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
For #23665 I'm about to make add a lot more code to the state store for the keyring, so I'd like to pull these out to their own file. Also updates the test to use `shoenig/test` and changes the name of one method to be a little more accurate.
tgross
force-pushed
the
keyring-refactor
branch
from
ad7bd51
to
cb3226f
Compare
tgross
requested review from
gulducat,
schmichael,
pkazmierczak and
Juanadelacuesta
pkazmierczak
approved these changes
Sep 12, 2024
| @@ -0,0 +1,196 @@ | |||
| // Copyright (c) HashiCorp, Inc. | |||
There was a problem hiding this comment.
bless your heart for moving this to a separate file 🙇♂️
| key = key.MakeInactive() | ||
| } | ||
| modified = true | ||
| case structs.RootKeyStateRekeying, structs.RootKeyStateDeprecated: |
There was a problem hiding this comment.
do we have to explicitly case this? why not just skip this line?
There was a problem hiding this comment.
It absolutely could be default: but when you've got a small enum like this I feel like it helps understandability to explicitly list them. If you add a state later it's more obvious that it's missing, rather than simply falling thru the default case and doing nothing.
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.
For #23665 I'm about to make add a lot more code to the state store for the keyring, so I'd like to pull these out to their own file. Also updates the test to use
shoenig/testand changes the name of one method to be a little more accurate.Note for reviewers:
nomad/encrypter.gointo its own package but right now that's got a lot of dependencies on server RPC code because of keyring replication and server lifecycle. So I'm punting on that here. Once I've got Store keyring in Raft #23665 ready to ship, I might revisit that decision but I don't want to push up a refactoring PR only to have to push up another one unwinding half of it once I'm done.