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

UPSTREAM: 45743: pick 45743 to fix config groupversion defaults #16976

Merged
merged 1 commit into from
Oct 22, 2017
Merged

UPSTREAM: 45743: pick 45743 to fix config groupversion defaults #16976

merged 1 commit into from
Oct 22, 2017

Conversation

juanvallejo
Copy link
Contributor

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1503910

Picks upstream changes introduced in https://github.com/kubernetes/kubernetes/pull/45743/files
which fix a bug where setting kubernetes defaults on a rest config would sometimes overwrite its ContentConfig.GroupVersion field with the wrong group version - causing resources such as cronjob.v2alpha1.batch to return an error [1] when attempted to be described through oc.

$ oc describe cronjob existing-cronjob
Error from server (NotFound): the server could not find the requested resource

cc @deads2k @fabianofranz

@openshift-merge-robot openshift-merge-robot added needs-api-review vendor-update Touching vendor dir or related files labels Oct 20, 2017
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 20, 2017
@juanvallejo
Copy link
Contributor Author

@deads2k this is not a clean pick - requires updating origin files such as login.go and cli/config/smart_merge.go since cluster.APIVersion is taken out as part of the upstream PR

@juanvallejo
Copy link
Contributor Author

@deads2k These are the only changes actually needed for the fix from the upstream PR:

https://github.com/openshift/origin/pull/16976/files#diff-ab133704268aa9a193886476b779d5e3
https://github.com/openshift/origin/pull/16976/files#diff-0b86e853cfdf42908b9d19fb15b7ad44

deads2k
deads2k reviewed Oct 20, 2017
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubectl/cmd/config/create_cluster.go Outdated
@@ -81,8 +80,6 @@ func NewCmdConfigSetCluster(out io.Writer, configAccess clientcmd.ConfigAccess)
options.insecureSkipTLSVerify.Default(false)

cmd.Flags().Var(&options.server, clientcmd.FlagAPIServer, clientcmd.FlagAPIServer+" for the cluster entry in kubeconfig")
cmd.Flags().Var(&options.apiVersion, clientcmd.FlagAPIVersion, "DEPRECATED: "+clientcmd.FlagAPIVersion+" for the cluster entry in kubeconfig")
Copy link
Contributor

Choose a reason for hiding this comment

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

when did we deprecate the flag? You may not be able to remove the altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

when did we deprecate the flag? You may not be able to remove the altogether.

Don't respect it, sure. But if we didn't deprecate in 3.6 or before, flag removal is naughty.

deads2k
deads2k reviewed Oct 20, 2017
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/clientcache.go
if err != nil {
return nil, err
}
config.GroupVersion = negotiatedVersion

if requiredVersion != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This pull looks vaguely familiar. Was it mine?

@deads2k
Copy link
Contributor

deads2k commented Oct 20, 2017

I feel like remember doing this. I don't know what all was left out of this, but the central idea looks fine.

@deads2k
Copy link
Contributor

deads2k commented Oct 20, 2017

/approve

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2017
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2017
@juanvallejo
Copy link
Contributor Author

@deads2k thanks, went ahead and only picked the relevant parts of the upstream PR that fix https://bugzilla.redhat.com/show_bug.cgi?id=1503910 - leaving all of the changes that deprecate / remove cluster.APIVersion out

@juanvallejo
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 20, 2017
@juanvallejo
Copy link
Contributor Author

/test extended_conformance_gce

@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Oct 21, 2017
@liggitt
Copy link
Contributor

liggitt commented Oct 21, 2017

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 008f2d5 into openshift:release-3.6 Oct 22, 2017
@juanvallejo juanvallejo deleted the jvallejo/pick-upstream-45743 branch October 23, 2017 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@liggitt liggitt

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@juanvallejo @deads2k @liggitt @openshift-merge-robot @openshift-bot @derekwaynecarr @openshift-ci-robot