Skip to content
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 the route field selectors #16305

Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 12, 2017

This makes the field selectors for routes work correctly and makes a slightly different pattern which works with the upstream defaulting and fieldkey methods so that we won't slip on object meta updates in the future. It also uses the actual scheme registration to determine if the field key conversion methods work.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 12, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Sep 12, 2017

/assign soltysh
/assign mfojtik

@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Sep 12, 2017
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, but mostly lgtm

// which many of our older resources used.
func LegacyMetaV1FieldSelectorConversionWithName(label, value string) (string, string, error) {
switch label {
case "name":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we're not handling namespace similarly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we're not handling namespace similarly?

It namespace was never special cased in a field selector.

@@ -50,7 +65,10 @@ func TestRoundTripVersionedObject(t *testing.T) {
}

func TestFieldSelectors(t *testing.T) {
testutil.CheckFieldLabelConversions(t, "v1", "ImageStream",
converter := runtime.NewScheme()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: scheme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: scheme

it is getting used as a converter, not a scheme. Scheme happens to implement converter.


_ "github.com/openshift/origin/pkg/api/install"
// some side-effect of this import is causing TestRoundTripVersionedObject to pass. I don't see it.
_ "github.com/openshift/origin/pkg/image/apis/image/install"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're manually creating the scheme do you still need to install the api?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're manually creating the scheme do you still need to install the api?

I found the side-effect last night, but I haven't found a good solution yet. For now, it has to remain like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely 😉

@@ -565,3 +564,11 @@ func GetSafeRouteName(name string) string {
}
return name
}

// TODO this should probably be removed and replaced with an upstream keyfunc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 mind creating an issue and assigning this to networking team?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And since this is only template and route and you're touching it, would you mind :) ?

sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind creating an issue and assigning this to networking team?

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -37,7 +36,7 @@ func ValidateProcessedTemplate(template *templateapi.Template) field.ErrorList {

// ValidateTemplate tests if required fields in the Template are set.
func ValidateTemplate(template *templateapi.Template) (allErrs field.ErrorList) {
allErrs = validation.ValidateObjectMeta(&template.ObjectMeta, true, oapi.GetNameValidationFunc(validation.ValidatePodName), field.NewPath("metadata"))
allErrs = validation.ValidateObjectMeta(&template.ObjectMeta, true, validation.ValidatePodName, field.NewPath("metadata"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, we should probably stop using ValidatePodName and create our own, as in ValidateTemplateName pointing to what that ValidatePodName is pointing, which is NameIsDNSSubdomain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And since this is only template and route and you're touching it, would you mind :) ?

apihelpers.GetFieldLabelConversionFunc(routeapi.RouteToSelectableFields(&routeapi.Route{}), nil),
)
func addLegacyFieldConversionFuncs(scheme *runtime.Scheme) error {
if err := scheme.AddFieldLabelConversionFunc(LegacySchemeGroupVersion.String(), "Route", legacyRouteFieldSelectorConversionFunc); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return scheme.AddField...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return scheme.AddField...

Are you sure? This stanza is copy/pasteable as more Kinds are added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still can be, and in most cases is the last line in addConversionFuncs. Above you'll keep adding type-specific conversion funcs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still can be, and in most cases is the last line in addConversionFuncs. Above you'll keep adding type-specific conversion funcs.

Eh, it really can't be. Conversions aren't tied to a particular external schema version, but field conversions are. That's why there are two different methods.

}

func addFieldConversionFuncs(scheme *runtime.Scheme) error {
if err := scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.String(), "Route", routeFieldSelectorConversionFunc); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

"spec.to.name":
return label, value, nil
default:
return runtime.DefaultMetaV1FieldSelectorConversion(label, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the other pr adding this to the rest, this struck me. Shouldn't that be return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the other pr adding this to the rest, this struck me. Shouldn't that be return apihelpers.LegacyMetaV1FieldSelectorConversionWithName(label, value) ?

Interestingly enough, no. The "name" -> "metadata.name" mapping never existed for routes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 15, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Sep 15, 2017

/retest

@soltysh
Copy link
Contributor

soltysh commented Sep 15, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, soltysh

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16089, 16305, 16219, 15934, 16366)

@openshift-merge-robot openshift-merge-robot merged commit ca68fa4 into openshift:master Sep 16, 2017
openshift-merge-robot added a commit that referenced this pull request Sep 16, 2017
Automatic merge from submit-queue (batch tested with PRs 16384, 16327, 16199, 16286, 16378)

fix remaining field selectors 

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.
@deads2k deads2k deleted the route-02-field-selector branch January 24, 2018 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants