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

remove Categories from /oapi resources #17583

Closed
wants to merge 3 commits into from
Closed

remove Categories from /oapi resources #17583

wants to merge 3 commits into from

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Dec 4, 2017 •
edited
Loading

Fixes #17446

Removes the Categories method from /oapi resources, introduced in #16823

Suggested in #17446 (comment)

cc @deads2k @openshift/cli-review @mfojtik @smarterclayton

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 4, 2017
@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo
Copy link
Contributor Author

/test extended_conformance_gce

@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install

1 similar comment
@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Dec 6, 2017 •
edited
Loading

@fabianofranz or @deads2k friendly ping

@fabianofranz
Copy link
Member

/assign @deads2k

@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2017

Please add a test to test-integration that creates the discovery category builder and checks to make sure it contains the groupified resources for openshift and kube that we expect.

@deads2k
Copy link
Contributor

deads2k commented Dec 15, 2017

Also, I'm fairly sure this breaks the groupified discovery as well.

@deads2k
Copy link
Contributor

deads2k commented Dec 19, 2017

@juanvallejo bump

@juanvallejo juanvallejo force-pushed the jvallejo/remove-categories-from-oapi-resources branch from 8e69ec8 to f316d99 Compare December 19, 2017 21:10
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2017
@juanvallejo
Copy link
Contributor Author

@deads2k thanks, added an integration test

@juanvallejo juanvallejo force-pushed the jvallejo/remove-categories-from-oapi-resources branch 2 times, most recently from 2d8bac8 to df33c22 Compare December 19, 2017 21:15
@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo juanvallejo force-pushed the jvallejo/remove-categories-from-oapi-resources branch 2 times, most recently from 4513d2f to be3674e Compare December 19, 2017 22:18
@juanvallejo juanvallejo force-pushed the jvallejo/remove-categories-from-oapi-resources branch from be3674e to e68de6d Compare December 19, 2017 23:19
@juanvallejo juanvallejo force-pushed the jvallejo/remove-categories-from-oapi-resources branch from 2d40465 to 84d75c7 Compare December 19, 2017 23:36
juanvallejo
juanvallejo commented Dec 19, 2017
View reviewed changes
test/integration/category_builder_test.go
"HorizontalPodAutoscaler/autoscaling/v2beta1": {"all"},
"Job/batch/v1": {"all"},
"DeploymentConfig/apps.openshift.io/v1": {"all"}, // FIX: discovered with no categories
"BuildConfig/build.openshift.io/v1": {"all"}, // FIX: discovered with no categories
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k I believe I am removing categories from the wrong resources. I can restore the Categories() method in the files above, however per the results of this test, /oapi resources continue to be requested as part of oc get all despite being discovered in this test without any categories

@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/openshift-jenkins/integration 84d75c7 link /test integration
ci/openshift-jenkins/cmd 84d75c7 link /test cmd
ci/openshift-jenkins/unit 84d75c7 link /test unit
ci/openshift-jenkins/extended_conformance_gce 84d75c7 link /test extended_conformance_gce
ci/openshift-jenkins/extended_conformance_install 84d75c7 link /test extended_conformance_install
ci/openshift-jenkins/extended_networking_minimal 84d75c7 link /test extended_networking_minimal

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.

@juanvallejo
Copy link
Contributor Author

Closing this in favor of #18085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhgupta abhgupta Awaiting requested review from abhgupta

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@deads2k deads2k

Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juanvallejo @fabianofranz @deads2k @openshift-ci-robot