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.

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

UPSTREAM: <carry>: hack out the oapi for restmapping resources when m… #18377

Merged
merged 2 commits into from
Feb 2, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 31, 2018

…ore than one is present

If there is more than one kind or resource for a value (the CLI uses this for deciding what to do about deploymentconfigs as a for instance), anything in that list that is an openshift resource (I've only listed one to prove the concept), will be removed.

I think that referencing names from a file will still work since that should come back with one match.

@juanvallejo can you see if files containing oapi resources still work.
@soltysh @mfojtik @smarterclayton is this a thing we can live with? It will finally make the groupified resources have priority on the CLI.
@DirectXMan12 I really wish this had not worked.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 31, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 31, 2018
var oapiResources = map[schema.GroupVersionResource]bool{
{Resource: "deploymentconfig"}: true,
{Resource: "deploymentconfigs"}: true,
{Resource: "dc"}: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's less than 100 of these

Copy link
Contributor

@mfojtik mfojtik Feb 1, 2018

Choose a reason for hiding this comment

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

can we make this pluggable in upstream, so you can register "ignored" resources like:

type predicateFunc func(gvk) bool
var IgnoredResources = map[schema.GroupVersionResource]predicateFunc{}

// then in origin we will have something like:

meta.IgnoredResources["deploymentconfig"] = func(gvk) { len(gvk.Group) == 0 }

Copy link
Contributor

Choose a reason for hiding this comment

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

(just thinking loud to avoid carry patch ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we make this pluggable in upstream, so you can register "ignored" resources like

I don't think there's a use-case upstream.

@enj
Copy link
Contributor

enj commented Jan 31, 2018

Uh this is the only way to force our groupified resources to be used?

@deads2k
Copy link
Contributor Author

deads2k commented Jan 31, 2018

Uh this is the only way to force our groupified resources to be used?

Fully open to other pulls. Priority is based on groupversion. The ""/v1 group/version is always first because the core API should win naming conflicts. Everything else is based on discovery order. We have hack in discovery that returns /oapi resources when the group/version ""/v1 is requested because /oapi resources have ""/v1 as their group/version (see yaml).

We have a special case for 75 or so resources in ""/v1 that they need to be behind the groupified API in priority. I think this is the simplest way to make it happen. I'm open to reasonable-ish alternatives.

@enj
Copy link
Contributor

enj commented Jan 31, 2018

I cannot think of a better solution off the top of my head (I expect any solution to be some variation on this hack in the end).

@mfojtik
Copy link
Contributor

mfojtik commented Feb 1, 2018

/retest

@mfojtik
Copy link
Contributor

mfojtik commented Feb 1, 2018

(flake was the router/prometheus )

@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 Feb 1, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Feb 1, 2018

now with our 75 or so resources

@deads2k deads2k changed the title [DO NOT MERGE] UPSTREAM: <carry>: hack out the oapi for restmapping resources when m… UPSTREAM: <carry>: hack out the oapi for restmapping resources when m… Feb 1, 2018
@enj
Copy link
Contributor

enj commented Feb 1, 2018

/lgtm

sadness

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@deads2k
Copy link
Contributor Author

deads2k commented Feb 1, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 18390, 18389, 18290, 18377, 18385).

@openshift-merge-robot openshift-merge-robot merged commit 1fef218 into openshift:master Feb 2, 2018
openshift-merge-robot added a commit that referenced this pull request Feb 9, 2018
…lectric-boogaloo

Automatic merge from submit-queue.

UPSTREAM: <carry>: Short-circuit HPA oapi/v1.DC

The legacy oapi v1 group-version very much confuses anything not
designed to explicitly work with it.  Since we now don't do any custom
HPA setup, we need to teach the scale client and the HPA what to do
with the oapi version of DC, since it won't even show up in its
discovery process.

Related to #18377
openshift-merge-robot added a commit that referenced this pull request Mar 6, 2018
Automatic merge from submit-queue (batch tested with PRs 18666, 18810, 18430, 18517, 18653).

Add migrate command for legacy HPAs

There are current broken HPAs floating around that either use the legacy
oapi DeploymentConfig defintion (`v1.DeploymentConfig`) or the incorrect
API group, due to the webconsole (all scalables, but with the group as
`extensions/v1beta1`).  This introduces a migrate command that corrects
the ScaleTargetRef of those HPAs to have correct API groups that are
usable by generic scale clients.

Related to #18377, #18380, openshift/origin-web-console#2776

cc @deads2k @liggitt @spadgett
@deads2k deads2k deleted the cli-18-priority branch July 3, 2018 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants