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

Use discovery based version gating for policy commands #16219

Merged
merged 1 commit into from
Sep 16, 2017

Conversation

mrogers950
Copy link
Contributor

This PR switches from the policy command version gating with hard-coded 3.7 versions to a solution based on the discovery of supported server resources.
Fixes #15831

@mrogers950
Copy link
Contributor Author

@openshift/cli-review

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

simo5 commented Sep 8, 2017

The mechanism looks good, however it is too hardcoded as built.
I think ResourceGate should get the name of the resources to gate for as input, and leave the function more generic, so it can be reused for other future gating.

if len(minServerVersion) == 0 && len(maxServerVersion) == 0 {
return fmt.Errorf("No version info passed to gate command")
// Do a discovery to see if the legacy policy resources are supported. If they are, return nil, otherwise return err.
func ResourceGate(ocClient *client.Client) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

add input parameters for group version and resource nmes here

@mrogers950
Copy link
Contributor Author

@simo5 updated.

simo5
simo5 previously requested changes Sep 8, 2017
}

// Do a discovery for server resources of the provided groupVersion, returning nil if any one of resourceNames is supported
// by the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that we should check if all of the resources are availble, not any, as in most case API are not actually interchangable. Otherwise we force callers to only pass one resource at a time and make multiple calls to make sure all the resources they need are available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bonus points would be if we return the list of unavailable ones with the error code, when some are unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it could do []GVR, err where err != nil is always fatal, and then LegacyPolicyResourceGate could wrap that with and extra error on a non-empty []GVR.

enj
enj previously requested changes Sep 8, 2017

// Do a discovery for server resources of the provided groupVersion, returning nil if any one of resourceNames is supported
// by the server.
func ResourceGate(ocClient *client.Client, groupVersion string, resourceNames []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should take a variatric list of GVR structs, not []string and a separate GV.

})
}

// Do a discovery for server resources of the provided groupVersion, returning nil if any one of resourceNames is supported
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to start with ResourceGate

if len(minServerVersion) == 0 && len(maxServerVersion) == 0 {
return fmt.Errorf("No version info passed to gate command")
// Return err if the server does not support any of the legacy policy objects (< 3.7)
func LegacyPolicyResourceGate(ocClient *client.Client) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to start with LegacyPolicyResourceGate. Turn on the inspection in Gogland.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be changed to take the client.Interface instead? Then you can actually add some unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to take just the discovery interface, which would be preferred.

return fmt.Errorf("No version info passed to gate command")
// Return err if the server does not support any of the legacy policy objects (< 3.7)
func LegacyPolicyResourceGate(ocClient *client.Client) error {
return ResourceGate(ocClient, "authorization.openshift.io/v1", []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Need both "" and "authorization.openshift.io" so 8 GVR items in the list total.

}

ocVersionBody, err := ocClient.Get().AbsPath("/version/openshift").Do().Raw()
serverVersion, err := ocClient.Discovery().ServerVersion()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really useful.

}

// Do a discovery for server resources of the provided groupVersion, returning nil if any one of resourceNames is supported
// by the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it could do []GVR, err where err != nil is always fatal, and then LegacyPolicyResourceGate could wrap that with and extra error on a non-empty []GVR.

@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 Sep 13, 2017
@mrogers950 mrogers950 force-pushed the discovery_gating branch 2 times, most recently from a7064b7 to e085cbc Compare September 14, 2017 12:59
@mrogers950
Copy link
Contributor Author

Updated with a bunch of changes from irl feedback from @enj. The gating is split into generic discovery and legacy gating functions, with added cmd and unit tests.

@soltysh
Copy link
Contributor

soltysh commented Sep 14, 2017

/unassign
/assign @enj

@openshift-ci-robot openshift-ci-robot assigned enj and unassigned soltysh Sep 14, 2017
@mrogers950
Copy link
Contributor Author

/retest

@mrogers950
Copy link
Contributor Author

flake #16144

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 14, 2017
@mrogers950
Copy link
Contributor Author

Updated from the last bit of @enj feedback (added tests for the DiscoverGroupVersionResources() GroupVersion caching)

@mrogers950
Copy link
Contributor Author

/retest

@mrogers950 mrogers950 dismissed stale reviews from simo5 and enj September 15, 2017 13:17

addressed changes from in-person feedback

@mrogers950
Copy link
Contributor Author

/retest

@simo5
Copy link
Contributor

simo5 commented Sep 15, 2017

/lgtm

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

enj commented Sep 15, 2017

/lgtm

👍 for adding another extremely well tested command.

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, mrogers950, simo5
We suggest the following additional approver: fabianofranz

Assign the PR to them by writing /assign @fabianofranz 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

@enj enj added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017
@enj
Copy link
Contributor

enj commented Sep 15, 2017

Approving since @juanvallejo said he was happy earlier and we need this ASAP.

@mrogers950
Copy link
Contributor Author

flake: #16369

/test extended_conformance_install_update

jupierce added a commit that referenced this pull request Sep 15, 2017
…stage

Use discovery based version gating for policy commands #16219
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16089, 16305, 16219, 15934, 16366)

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants