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

Make thirdparty codec able to decode DeleteOptions #37328

Merged
merged 1 commit into from Dec 6, 2016
Merged

Make thirdparty codec able to decode DeleteOptions #37328

merged 1 commit into from Dec 6, 2016

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Nov 23, 2016 •
edited by k8s-oncall

Fix #37278.

Without this PR, the gvk sent to the delegated codec will be the thirdparty one, which is not recognized by the delegated codec (usually api.Codecs).

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Nov 23, 2016
@caesarxuchao
Copy link
Member Author

@deads2k @smarterclayton could you help review it?

@caesarxuchao caesarxuchao added cherrypick-candidate release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 29, 2016
@caesarxuchao caesarxuchao reopened this Nov 29, 2016
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@smarterclayton
Copy link
Contributor

I wonder why gvk was set as the default - doesn't make much sense.

@caesarxuchao
Copy link
Member Author

@brendandburns is it intentional to set gvk as the default? You are the original author. Thank.

@smarterclayton
Copy link
Contributor

Oh, I see what is happening. You need to check the defaulting GVK (if not nil) and send it onwards only if it doesn't point to your group.

if gvk != nil && gvk.Group == TPR_GROUP {
  gvk = nil
}
d.delegate.Decode(...)

In this case you know that the nested delegate cannot recognize your group (the TPR group) so no defaulting should be possible. That's the safer change, since defaulting for something like a v1.Status decode should still work.

@caesarxuchao
Copy link
Member Author

You are right. Updated. PTAL. Thanks.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 58e8fde5e68298abe48dc06cb82d24b90450f87e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 58e8fde5e68298abe48dc06cb82d24b90450f87e. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 58e8fde5e68298abe48dc06cb82d24b90450f87e. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 58e8fde5e68298abe48dc06cb82d24b90450f87e. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit 58e8fde5e68298abe48dc06cb82d24b90450f87e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@caesarxuchao
Copy link
Member Author

@k8s-bot kubemark e2e test this
(stuck with "waiting for status..")

@caesarxuchao
Copy link
Member Author

@k8s-bot cvm gce e2e test this
(stuck with "waiting for status..")

@dims
Copy link
Member

dims commented Nov 30, 2016

@k8s-bot kubemark e2e test this

@dims
Copy link
Member

dims commented Nov 30, 2016

@k8s-bot cvm gce e2e test this

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit c0e3790f848b6987ee1ef4c0269d5884ff7d7145. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit c0e3790f848b6987ee1ef4c0269d5884ff7d7145. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2016
@caesarxuchao
Copy link
Member Author

Rebased. Didn't change anything. Adding back the lgtm.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 5, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 6534661. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@caesarxuchao
Copy link
Member Author

@k8s-bot gce etcd3 e2e test this, issue #27680

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 37328, 38102, 37261, 31321, 38146)

@k8s-github-robot k8s-github-robot merged commit b1dba6c into kubernetes:master Dec 6, 2016
This was referenced Jan 5, 2017
Merged
Merged
@sttts sttts added this to the v1.5 milestone Jan 10, 2017
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jan 10, 2017
k8s-github-robot pushed a commit that referenced this pull request Jan 11, 2017
Merge pull request #39665 from zhouhaibing089/automated-cherry-pick-o…
8e08464 
…f-#37328-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #37328

Cherry pick of #37328 on release-1.5.

#37328: make thirdparty codec able to decode DeleteOptions
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@mengqiy mengqiy mentioned this pull request Mar 14, 2017
Closed
This was referenced Mar 28, 2017
Closed
Closed
databus23 added a commit to sapcc/kubernetes that referenced this pull request May 15, 2017
@databus23
Manual back-port kubernetes#37328 (Make thirdparty codec able to deco…
1800b8a 
…de DeleteOptions)

Hopefully this allows us to delete namespaces again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@caesarxuchao @k8s-cherrypick-bot @smarterclayton @k8s-ci-robot @dims @k8s-github-robot @lavalamp @sttts @saad-ali