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
Only default Job fields when feature gates are enabled #100188
Conversation
@alculquicondor: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions 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/test-infra repository. |
FYI @adtac |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
@@ -153,21 +153,10 @@ func testJobStrategy(t *testing.T) { | |||
t.Errorf("Expected a validation error") | |||
} | |||
|
|||
// Existing gated fields should be preserved |
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.
why was this removed?
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.
To keep the test simple. Other fields are already tested above.
It would be ideal to have a test table so that we don't have to repeat the checks, but kind of late to do such refactoring.
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 don't know if removing the TTL test is a good idea though
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 don't know if removing the TTL test is a good idea though
/hold
just saw this... agree that this should not change unrelated fields
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.
changes in TTL is already tested above
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.
ack
/hold cancel
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.
not for PrepareForUpdate though
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.
TTLSecondsAfterFinished: pointer.Int32Ptr(1), |
if !utilfeature.DefaultFeatureGate.Enabled(features.IndexedJob) && oldJob.Spec.CompletionMode == batch.NonIndexedCompletion { | ||
newJob.Spec.CompletionMode = batch.NonIndexedCompletion | ||
if !utilfeature.DefaultFeatureGate.Enabled(features.IndexedJob) && oldJob.Spec.CompletionMode == nil { | ||
newJob.Spec.CompletionMode = nil |
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.
this would allow updating from NonIndexed->Indexed even when the feature gate is disabled, no?
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.
It would... but field is immutable (checked in ValidateJobSpecUpdate
)
Also, the feature gate is, in a way, protecting the field to not be set, without caring about the value.
7a65a95
to
90fc9b9
Compare
90fc9b9
to
79c9d8f
Compare
/assign @liggitt |
79c9d8f
to
be3eb34
Compare
you need to update the API compatibility fixtures to v1.19 and v1.20 fixtures, which will effectively undo the changes made in #98441 In general, changes to the "after_roundtrip" fixtures from previous versions is a big red flag and should not be expected in PRs adding new API fields (e.g. https://github.com/kubernetes/kubernetes/pull/98441/files#diff-5c45c5bb5dd4a8b59eceff1c90f520425cac8fa64599ecf5d37f03da37ad6549) |
The API change itself lgtm |
be3eb34
to
ca0f828
Compare
Also use pointer for completionMode enum
ca0f828
to
e6c3d7b
Compare
/cc @palnabarun @nikhita |
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.
controller and test changes lgtm
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
What type of PR is this?
/kind bug
/kind api-change
What this PR does / why we need it:
Only default Job fields when feature gates are enabled and use pointer for completionMode enum.
This avoids rendering the field when the feature gates are disabled.
/sig apps
Which issue(s) this PR fixes:
Fixes #100187
kubernetes/enhancements#2214
kubernetes/enhancements#2232
Does this PR introduce a user-facing change?
Features not released yet