Skip to content

Prefer GroupVersionResource, fallback to GVK#60932

Merged
k8s-github-robot merged 1 commit intokubernetes:masterfrom
juanvallejo:jvallejo/fix-builder-mapping
Mar 9, 2018
Merged

Prefer GroupVersionResource, fallback to GVK#60932
k8s-github-robot merged 1 commit intokubernetes:masterfrom
juanvallejo:jvallejo/fix-builder-mapping

Conversation

@juanvallejo
Copy link
Copy Markdown
Contributor

@juanvallejo juanvallejo commented Mar 8, 2018

Release note:

NONE

Addresses #59353 (comment)

cc @smarterclayton @deads2k @soltysh

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 8, 2018
@k8s-ci-robot k8s-ci-robot requested review from deads2k and rootfs March 8, 2018 17:15
@juanvallejo juanvallejo changed the title Preferf GroupVersionResource, fallback to GVK Prefer GroupVersionResource, fallback to GVK Mar 8, 2018
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 8, 2018
@juanvallejo juanvallejo mentioned this pull request Mar 8, 2018
Merged
deads2k
deads2k reviewed Mar 8, 2018
View reviewed changes
pkg/kubectl/resource/builder.go Outdated
Copy link
Copy Markdown
Contributor

@deads2k deads2k Mar 8, 2018

Choose a reason for hiding this comment

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

if this errors out, you want to return return b.mapper.RESTMapping(gvk.GroupKind(), gvk.Version), right?

Copy link
Copy Markdown
Contributor Author

@juanvallejo juanvallejo Mar 8, 2018

Choose a reason for hiding this comment

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

Thanks, updated this

@juanvallejo juanvallejo force-pushed the jvallejo/fix-builder-mapping branch from 890108f to dbffd9c Compare March 8, 2018 19:15
@soltysh
Copy link
Copy Markdown
Contributor

soltysh commented Mar 8, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 8, 2018
soltysh
soltysh approved these changes Mar 8, 2018
View reviewed changes
Copy link
Copy Markdown
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.

One small nit, otherwise lgtm.

pkg/kubectl/resource/builder.go Outdated
Copy link
Copy Markdown
Contributor

@soltysh soltysh Mar 8, 2018

Choose a reason for hiding this comment

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

Given the current changes the godoc needs update as well.

Copy link
Copy Markdown
Contributor Author

@juanvallejo juanvallejo Mar 8, 2018

Choose a reason for hiding this comment

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

Thanks for catching that. Updated godoc to reflect new changes

@soltysh soltysh added this to the v1.10 milestone Mar 8, 2018
@soltysh soltysh added kind/bug Categorizes issue or PR as related to a bug. status/approved-for-milestone labels Mar 8, 2018
@soltysh soltysh added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Mar 8, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/fix-builder-mapping branch 2 times, most recently from 2acf5f6 to 67d535e Compare March 9, 2018 15:51
soltysh
soltysh approved these changes Mar 9, 2018
View reviewed changes
Copy link
Copy Markdown
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2018
@soltysh
Copy link
Copy Markdown
Contributor

soltysh commented Mar 9, 2018

/assign @smarterclayton
for approval
/retest

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Mar 9, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@juanvallejo @smarterclayton @soltysh

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@juanvallejo juanvallejo force-pushed the jvallejo/fix-builder-mapping branch from 67d535e to 81504a4 Compare March 9, 2018 19:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2018
@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Mar 9, 2018

/retest

smarterclayton
smarterclayton reviewed Mar 9, 2018
View reviewed changes
hack/make-rules/test-cmd-util.sh Outdated
Copy link
Copy Markdown
Contributor

@smarterclayton smarterclayton Mar 9, 2018

Choose a reason for hiding this comment

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

This message is technically incorrect - we couldn't find a resource or a kind called unknown resource type, and we are very careful in the CLI to describe this argument as a resource argument. I think you need to fix the message in the case where you fall all the way through to be consistent with the previous release.

Copy link
Copy Markdown
Contributor Author

@juanvallejo juanvallejo Mar 9, 2018

Choose a reason for hiding this comment

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

Thanks, added an error message when this happens to maintain consistency with previous behavior

@juanvallejo juanvallejo force-pushed the jvallejo/fix-builder-mapping branch from 81504a4 to 35d4fde Compare March 9, 2018 20:42
@juanvallejo juanvallejo force-pushed the jvallejo/fix-builder-mapping branch from 35d4fde to 177dcb9 Compare March 9, 2018 20:43
soltysh
soltysh approved these changes Mar 9, 2018
View reviewed changes
Copy link
Copy Markdown
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2018
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Mar 9, 2018

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2018
@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Mar 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Mar 9, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a2f0ddc into kubernetes:master Mar 9, 2018
@juanvallejo juanvallejo deleted the jvallejo/fix-builder-mapping branch March 9, 2018 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@deads2k deads2k deads2k left review comments

@soltysh soltysh soltysh approved these changes

@rootfs rootfs Awaiting requested review from rootfs

Assignees

@soltysh soltysh

@smarterclayton smarterclayton

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v1.10

Development

Successfully merging this pull request may close these issues.

6 participants

@juanvallejo @soltysh @k8s-github-robot @deads2k @k8s-ci-robot @smarterclayton