-
Notifications
You must be signed in to change notification settings - Fork 495
Simplified rollout triggers and CRD design doc #34959
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?
Simplified rollout triggers and CRD design doc #34959
Conversation
doc/developer/design/20260209_simplified_rollout_triggers_and_crd.md
Outdated
Show resolved
Hide resolved
doc/developer/design/20260209_simplified_rollout_triggers_and_crd.md
Outdated
Show resolved
Hide resolved
doc/developer/design/20260209_simplified_rollout_triggers_and_crd.md
Outdated
Show resolved
Hide resolved
doc/developer/design/20260209_simplified_rollout_triggers_and_crd.md
Outdated
Show resolved
Hide resolved
| - `environmentdScratchVolumeStorageRequirement` | ||
| - `serviceAccountName` | ||
| - `serviceAccountAnnotations` | ||
| - `serviceAccountLabels` |
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.
the service account annotations and labels are also applied immediately, so they probably shouldn't be here (although a change to serviceAccountName will require a rollout since that needs to update the corresponding field on the statefulset)
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.
I think they might still be load bearing, despite being applied immediately. If we change the annotations on the service account to add an AWS IAM role ARN for example, do the credentials get applied to existing pods? I'm not sure if they do.
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, i guess that's true. may be worth testing to see what the behavior here is, but i think you're probably right. we'll probably want to leave a comment explaining this, since it's not immediately obvious
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.
I've added a comment.
Simplified rollout triggers and CRD design doc
Motivation
Part of https://linear.app/materializeinc/issue/DEP-7/design-simplifying-upgrade-rollouts-node-rolls-converted-to-project
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.