Stop defaulting version ID on control plane creation.#4217
Stop defaulting version ID on control plane creation.#4217
Conversation
We have previously confirmed our desire to eliminate defaulting. It's time. This will probably break some e2es that we will find and fix, but pain to day is better than mistakes tomorrow. We'll finish filling in reality from cluster-service later.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // Version should be immutable once is created | ||
| // additional validations may depend on the subscription, hence they will be done in the admission package | ||
| // ID string `json:"id,omitempty" | ||
| errs = append(errs, validate.ImmutableByCompare(ctx, op, fldPath.Child("id"), &newObj.ID, safe.Field(oldObj, toVersionID))...) |
There was a problem hiding this comment.
Place behind AFEC until testing done? I'm open to it.
There was a problem hiding this comment.
/hold
this at least needs logic to prevent moving backwards.
There was a problem hiding this comment.
fixed
/hold cancel
| } | ||
|
|
||
| // ChannelGroup string `json:"channelGroup,omitempty"` | ||
| errs = append(errs, validate.ImmutableByCompare(ctx, op, fldPath.Child("channelGroup"), &newObj.ChannelGroup, safe.Field(oldObj, toChannelGroup))...) |
There was a problem hiding this comment.
we allow this in our controller logic and should allow it in general.
|
|
||
| // Version ID is required for non-stable channel groups | ||
| if newObj.ChannelGroup != "stable" { | ||
| errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("id"), &newObj.ID, nil)...) |
There was a problem hiding this comment.
now always required.
deads2k
changed the title
|
/hold thinking this through, when we allow version.ID to change we need to ensure it doesn't move past the most recent 4.y+1. I'm going to make it immutable again after the AFEC PR so we can test #4218 |
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
replaced by #4218 |
2 participants
We have previously confirmed our desire to eliminate defaulting. It's time. This will probably break some e2es that we will find and fix, but pain to day is better than mistakes tomorrow. We'll finish filling in reality from cluster-service later.