-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ccl/cliccl: avoid opening Engine in debug encryption-decrypt #120547
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
17d87b8
to
0b20a38
Compare
Adapt the `debug encryption-decrypt` command to avoid actually opening the Engine and instead only open the filesystem environment. This allows the command to be used even when missing or corrupt files prevent the Engine from being opened. Epic: none Fix cockroachdb#96699. Release note: none
0b20a38
to
11371ae
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)
pkg/ccl/cliccl/ear_test.go
line 163 at r1 (raw file):
cmd.SetOut(&b) cmd.SetErr(&b) require.NoError(t, cmd.Flags().Set("enterprise-encryption", encSpecStr))
I'm really perplexed as to how this test ever passed without this?
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)
pkg/ccl/cliccl/ear_test.go
line 163 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
I'm really perplexed as to how this test ever passed without this?
Ah, right, the file registry is not encrypted. I think this is a fine additional restriction to require the --enterprise-encryption
flag, but lemme know your thoughts
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.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)
pkg/ccl/cliccl/ear_test.go
line 163 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
Ah, right, the file registry is not encrypted. I think this is a fine additional restriction to require the
--enterprise-encryption
flag, but lemme know your thoughts
I don't really have an opinion
TFTR! bors r=sumeerbhola |
Build succeeded: |
Adapt the
debug encryption-decrypt
command to avoid actually opening the Engine and instead only open the filesystem environment. This allows the command to be used even when missing or corrupt files prevent the Engine from being opened.Epic: none
Fix #96699.
Release note: none