fix the route field selectors by deads2k · Pull Request #16305 · openshift/origin

deads2k · 2017-09-12T16:03:38Z

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.

deads2k · 2017-09-12T16:03:52Z

/assign soltysh
/assign mfojtik

soltysh

A few nits, but mostly lgtm

soltysh · 2017-09-14T10:33:30Z

pkg/api/apihelpers/fields.go

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


Why we're not handling namespace similarly?

Why we're not handling namespace similarly?

It namespace was never special cased in a field selector.

soltysh · 2017-09-14T10:41:36Z

pkg/image/apis/image/v1/conversion_test.go


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


nit: scheme

nit: scheme

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

soltysh · 2017-09-14T10:41:51Z

pkg/image/apis/image/v1/conversion_test.go


-	_ "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"


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

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.

Lovely 😉

soltysh · 2017-09-14T10:45:56Z

pkg/router/controller/ingress.go

 	return name
 }
+
+// TODO this should probably be removed and replaced with an upstream keyfunc


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

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

sure.

mind creating an issue and assigning this to networking team?

sure

soltysh · 2017-09-14T10:50:54Z

pkg/template/apis/template/validation/validation.go

 // 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"))


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

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

soltysh · 2017-09-14T10:55:09Z

pkg/route/apis/route/v1/conversion.go

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


return scheme.AddField...

return scheme.AddField...

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

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

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.

soltysh · 2017-09-14T10:55:30Z

pkg/route/apis/route/v1/conversion.go

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


soltysh · 2017-09-14T13:28:28Z

pkg/route/apis/route/v1/conversion.go

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


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

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.

deads2k · 2017-09-15T12:58:47Z

/retest

soltysh · 2017-09-15T13:11:46Z

/lgtm

openshift-merge-robot · 2017-09-15T13:11:58Z

[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.

Details

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [deads2k]
~~pkg/api/OWNERS~~ [deads2k]
~~pkg/route/OWNERS~~ [deads2k]
~~pkg/template/OWNERS~~ [deads2k,soltysh]

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 · 2017-09-16T03:09:38Z

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

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.

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

openshift-ci-robot assigned mfojtik and soltysh Sep 12, 2017

openshift-merge-robot assigned imcsk8 and pravisankar Sep 12, 2017

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

deads2k force-pushed the route-02-field-selector branch from daee968 to 02e0217 Compare September 12, 2017 16:33

deads2k mentioned this pull request Sep 13, 2017

fix remaining field selectors #16327

Merged

soltysh reviewed Sep 14, 2017

View reviewed changes

update route field selectors

ba743f0

deads2k force-pushed the route-02-field-selector branch from 02e0217 to ba743f0 Compare September 15, 2017 12:34

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

openshift-merge-robot removed the needs-api-review label Sep 15, 2017

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

soltysh approved these changes Sep 15, 2017

View reviewed changes

openshift-merge-robot merged commit ca68fa4 into openshift:master Sep 16, 2017

deads2k mentioned this pull request Sep 25, 2017

jenkins in minishift sample project fails to start and run builds openshift/jenkins-sync-plugin#173

Closed

deads2k deleted the route-02-field-selector branch January 24, 2018 14:34

Conversation

deads2k commented Sep 12, 2017

Uh oh!

deads2k commented Sep 12, 2017

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Sep 15, 2017

Uh oh!

soltysh commented Sep 15, 2017

Uh oh!

openshift-merge-robot commented Sep 15, 2017

Uh oh!

openshift-merge-robot commented Sep 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants