-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
fix remaining field selectors #16327
fix remaining field selectors #16327
Conversation
[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.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
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.
A few nits, mostly lgtm.
|
||
func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error { | ||
return 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.
Why this is empty?
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 this is empty?
We don't have any custom field selectors. Yet. I'm placing the skeleton here so people can more easily figure out what to do. This isn't really obvious.
|
||
if err := scheme.AddFieldLabelConversionFunc("v1", "BuildConfig", apihelpers.LegacyMetaV1FieldSelectorConversionWithName); err != nil { | ||
func addFieldSelectorKeyConversions(scheme *runtime.Scheme) error { | ||
if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Build", buildFieldSelectorKeyConversionFunc); err != nil { | ||
return err | ||
} |
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.
No BC here?
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.
No BC here?
No. BC only supports metadata, but we have the "name" -> "metadata.name" mapping for the ungroupified versions.
"user.uid": | ||
return label, value, nil | ||
default: | ||
return runtime.DefaultMetaV1FieldSelectorConversion(label, value) |
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.
return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value)
?
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.
return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value)?
This one never had the "name" -> "metadata.name" mapping.
b0282d0
to
82c48ea
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.
This lgtm, but since this also contains changes from #16305 let's wait for the other to merge before applying the label.
/retest |
} | ||
fieldSet["spec.dockerImageRepository"] = imageStream.Spec.DockerImageRepository | ||
fieldSet["status.dockerImageRepository"] = imageStream.Status.DockerImageRepository |
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.
@deads2k we now also have status.publicDockerImageRepository if that matters
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.
@deads2k we now also have status.publicDockerImageRepository if that matters
Doesn't look like it's used as a field selector.
Automatic merge from submit-queue (batch tested with PRs 16384, 16327, 16199, 16286, 16378) |
Really hoping that #16305 works out. This updates all the rest for that pattern and allows us to remove a ton of boilerplate while maintaining decent unit test coverage. Still need a "you forgot to add a test" test, but that was already missing.