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 origin types kubectl scheme #17815

Closed
wants to merge 2 commits into from
Closed

Add origin types kubectl scheme #17815

wants to merge 2 commits into from

Conversation

juanvallejo
Copy link
Contributor

Fixes #17793

cc @deads2k

@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Dec 14, 2017
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 14, 2017
@deads2k
Copy link
Contributor

deads2k commented Dec 14, 2017

See travis for the revert commit format

deads2k
deads2k reviewed Dec 14, 2017
View reviewed changes
cmd/oc/oc.go Outdated
@@ -31,6 +46,20 @@ func main() {
runtime.GOMAXPROCS(runtime.NumCPU())
}

// add Origin types to the kubectl scheme
apps.Install(scheme.GroupFactoryRegistry, scheme.Registry, scheme.Scheme)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make an InstallAll method in pkg/api/install

@deads2k
Copy link
Contributor

deads2k commented Dec 14, 2017

right idea. Nit on commit name and where the big-button install lives.

@juanvallejo juanvallejo force-pushed the jvallejo/add-origin-types-kubectl-scheme branch from 9e51498 to dbdb9bb Compare December 14, 2017 21:33
@juanvallejo
Copy link
Contributor Author

@deads2k thanks, added an InstallAll method

@juanvallejo
Copy link
Contributor Author

/retest

deads2k
deads2k reviewed Dec 15, 2017
View reviewed changes
cmd/oc/oc.go Outdated
@@ -13,7 +13,7 @@ import (
"github.com/openshift/origin/pkg/oc/cli"

// install all APIs
_ "github.com/openshift/origin/pkg/api/install"
api "github.com/openshift/origin/pkg/api/install"
Copy link
Contributor

Choose a reason for hiding this comment

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

apinstall

Copy link
Contributor

Choose a reason for hiding this comment

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

apiinstall

deads2k
deads2k reviewed Dec 15, 2017
View reviewed changes
pkg/api/install/install.go Outdated
@@ -592,3 +594,19 @@ func init() {
return false, nil
})
}

func InstallAll() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should take a scheme as an argument

@juanvallejo juanvallejo force-pushed the jvallejo/add-origin-types-kubectl-scheme branch from dbdb9bb to bf22e71 Compare December 15, 2017 18:34
@juanvallejo
Copy link
Contributor Author

@deads2k thanks, review comments addressed

deads2k
deads2k reviewed Dec 15, 2017
View reviewed changes
pkg/api/install/install.go Outdated
@@ -592,3 +596,19 @@ func init() {
return false, nil
})
}

func InstallAll(scheme *runtime.Scheme, groupFactoryRegistry announced.APIGroupFactoryRegistry, registry *registered.APIRegistrationManager) {
// add Origin types to the kubectl scheme
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment is now wrong

@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2017

nit, squash last two commits, then tag at will.

@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2017

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/add-origin-types-kubectl-scheme branch from bf22e71 to 8506a46 Compare December 15, 2017 18:52
@juanvallejo juanvallejo added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/add-origin-types-kubectl-scheme branch from 8506a46 to d2882a1 Compare December 15, 2017 21:10
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2017
@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2017
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@juanvallejo
Copy link
Contributor Author

@deads2k I am seeing the test-cmd failure locally as well. Not sure what the cause of it might be:

test/cmd/images.sh:301: executing 'oc get is ruby -o jsonpath={.spec.tags[*].name}' expecting success and text '2.0 2.2 2.3 2.4 latest newtag'
FAILURE after 0.232s: test/cmd/images.sh:301: executing 'oc get is ruby -o jsonpath={.spec.tags[*].name}' expecting success and text '2.0 2.2 2.3 2.4 latest newtag': the output content test failed
Standard output from the command:
2.4 newtag

@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

@juanvallejo try against master. My guess is that its this pull.

@juanvallejo
Copy link
Contributor Author

@deads2k looks like even with the InstallAll method being called, using kubectl scheme instead of legacyscheme in apply causes an IsNotRegisteredError here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/apply.go#L612 which falls back to patching an object using a non-strategic merge patch.

This is the working strategic three-way merge patch that is generated when using legacyscheme:
http://pastebin.test.redhat.com/541776

This is the broken patch that is generated when switching to kubectl/scheme (with the changes from this PR):
http://pastebin.test.redhat.com/541779

@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

@deads2k looks like even with the InstallAll method being called, using kubectl scheme instead of legacyscheme in apply causes an IsNotRegisteredError here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/apply.go#L612 which falls back to patching an object using a non-strategic merge patch.

Probably need to figure out which object and why. That doesn't match my expectation.

@juanvallejo
Copy link
Contributor Author

Probably need to figure out which object and why. That doesn't match my expectation.

It's an ImageStream: http://pastebin.test.redhat.com/541789
being patched with: http://pastebin.test.redhat.com/541786

Will look into why

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot
Copy link

@juanvallejo: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/cmd d2882a1 link /test cmd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@deads2k deads2k mentioned this pull request Dec 19, 2017
Merged
@juanvallejo
Copy link
Contributor Author

closing in favor of #17885

openshift-merge-robot added a commit that referenced this pull request Dec 20, 2017
@openshift-merge-robot
Merge pull request #17885 from deads2k/api-14-legacyinstall
499023e 
Automatic merge from submit-queue (batch tested with PRs 17859, 17885).

Add origin types kubectl scheme 

Fixes #17793 

Builds on #17815

/assign sttts
/assign juanvallejo

This centralizes the legacy install function. The next logical step is going to require that we start plumbing particular schemes or build individual schemes.  I looked at doing that here, but it appears we have some 500 hits just for scheme usage and I haven't decided the optimal solution for something like our graph yet.
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

@liggitt liggitt Awaiting requested review from liggitt

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@deads2k deads2k

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/M Denotes a PR that changes 30-99 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.
5 participants
@juanvallejo @deads2k @openshift-ci-robot @openshift-bot @openshift-merge-robot