-
Notifications
You must be signed in to change notification settings - Fork 468
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
Teach upgrade checker to use aws-secrets-reader #29792
Teach upgrade checker to use aws-secrets-reader #29792
Conversation
804aaa9
to
1541762
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.
If it winds up being possible to pass in the deploy generation on the cloud side, can you also revert #29765 in this PR?
1541762
to
3f78fbf
Compare
3f78fbf
to
912ad78
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 modulo build failures. Thanks again for handling all 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.
Looks good! Can you make sure to address my one comment though before merging?
@@ -1088,7 +1088,7 @@ impl UnopenedPersistCatalogState { | |||
.into()); | |||
} | |||
( | |||
Mode::Writable, | |||
Mode::Writable | Mode::Savepoint, |
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.
Was it necessary to reintroduce this? Maybe this got pulled in as part of a rebase? Joe removed it very recently 76bdf33
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.
Ah this was intentional—see #29792 (review)!
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.
AFAICT, 76bdf33 was a hack to work around passing in the correct deploy generation, as we're setting up to do with this PR.
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.
Yes, I believe joe introduced this because it broke the upgrade checker... I fixed the upgrade checker :)
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.
Ah ha! Makes sense :)
Teach upgrade checker to use aws-secrets-reader
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.