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.

Sign up for GitHub

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

Jump to bottom

add legacy field selector for names #15963

Merged
merged 1 commit into from
Aug 26, 2017
Merged

add legacy field selector for names #15963

merged 1 commit into from
Aug 26, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 24, 2017

I think this gets me down the dozen or so "real" field selectors I need to convert for groupification.

@openshift-merge-robot openshift-merge-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 24, 2017
@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2017
@bparees
Copy link
Contributor

bparees commented Aug 24, 2017

/unassign

@deads2k deads2k force-pushed the api-05-legacyfield branch from b45f718 to 11c3dc9 Compare August 24, 2017 18:13
@deads2k
Copy link
Contributor Author

deads2k commented Aug 24, 2017

@enj since you like removing boiilerplate

ironcladlou
ironcladlou suggested changes Aug 24, 2017
View reviewed changes
Copy link
Contributor

@ironcladlou ironcladlou left a comment

Choose a reason for hiding this comment

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

Sorry for all the duplicate comments- seems likely there's a single explanation for most of them but I wanted to make sure

pkg/deploy/apis/apps/v1/conversion.go
oapi.GetFieldLabelConversionFunc(newer.DeploymentConfigToSelectableFields(&newer.DeploymentConfig{}), nil),
); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required for buildconfigs but not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't have a special case to map "name" to "metadata.name" for field selection.

pkg/image/apis/image/v1/conversion.go
); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required for buildconfigs but not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't have name -> metadata.name

pkg/image/apis/image/v1/conversion.go
); err != nil {
return err
}

if err := scheme.AddFieldLabelConversionFunc("v1", "ImageStream",
oapi.GetFieldLabelConversionFunc(newer.ImageStreamToSelectableFields(&newer.ImageStream{}), map[string]string{"name": "metadata.name"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing port to the new LegacyMetaV1FieldSelectorConversionWithName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing port to the new LegacyMetaV1FieldSelectorConversionWithName?

didn't forget this one. This field selector actually lists many fields so the conversion can't be done via a straight default. It will require more thought and code.

pkg/oauth/apis/oauth/v1/conversion.go
); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required for buildconfigs but not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't do name->metadata.name

pkg/user/apis/user/v1/conversion.go
); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required for buildconfigs but not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't have name->metadata.name

pkg/user/apis/user/v1/conversion.go
oapi.GetFieldLabelConversionFunc(userapi.UserToSelectableFields(&userapi.User{}), nil),
); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required for buildconfigs but not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't have name->metadata.name

@ironcladlou
Copy link
Contributor

That add/remove ratio...

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@deads2k deads2k force-pushed the api-05-legacyfield branch from 11c3dc9 to b6b8323 Compare August 25, 2017 12:14
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2017
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017
@deads2k deads2k force-pushed the api-05-legacyfield branch from b6b8323 to d14cfb5 Compare August 25, 2017 14:02
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 25, 2017
@deads2k deads2k added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm Indicates that a PR is ready to be merged. labels Aug 25, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Aug 25, 2017

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Aug 25, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15904, 15962, 15838, 15965, 15963)

@openshift-merge-robot openshift-merge-robot merged commit caa7167 into openshift:master Aug 26, 2017
@smarterclayton smarterclayton mentioned this pull request Aug 27, 2017
Closed
@deads2k deads2k mentioned this pull request Sep 25, 2017
Closed
@deads2k deads2k deleted the api-05-legacyfield branch January 24, 2018 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ironcladlou ironcladlou ironcladlou approved these changes

Assignees

@ironcladlou ironcladlou

@soltysh soltysh

@enj enj

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@deads2k @bparees @ironcladlou @openshift-merge-robot @openshift-bot @soltysh @enj @openshift-ci-robot