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

Clean policy #17491

Closed
wants to merge 1 commit into from
Closed

Clean policy #17491

wants to merge 1 commit into from

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Nov 28, 2017

This is one of 1.8 post-rebase tasks. It includes:

  • reformatting so it's easier to compare with upstream roles
  • removes networkpolicies from extensions, since they moved to own group. What worries me here is that I don't see any networkpolicy-related role in upstream, so I'm not sure if we might be bitten by it @openshift/sig-networking any ideas why we had read/write access for extensions.networkpolicies?

@deads2k || @liggitt ptal
/cc @openshift/sig-security

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh
We suggest the following additional approver: mfojtik

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

@simo5
Copy link
Contributor

simo5 commented Nov 28, 2017

I see other changes besides networkpolicies, but it is hard to follow whatthey are due to the mixing of reformatting and other changes.
for review it would be much better to split commits so that each intentional change is in its own commit with a comment that says why and what is being done.

@danwinship
Copy link
Contributor

removes networkpolicies from extensions, since they moved to own group.

Is that correct, compatibility-wise? In 1.8/3.8 extensions.NetworkPolicy is still the "default" type. (eg, if you "oc get networkpolicies", you get back extensions.NetworkPolicies, not networking.NetworkPolicies)

What worries me here is that I don't see any networkpolicy-related role in upstream, so I'm not sure if we might be bitten by it

"admin" should definitely have read/write access to NetworkPolicy. I guess we didn't add that in OpenShift until after all the role stuff got upstreamed, but I was still thinking of roles as being OpenShift-only at that point so it didn't occur to me to fix it upstream too. I can do a PR once we figure out for sure if it's just networking or both networking and extensions.

@enj
Copy link
Contributor

enj commented Nov 28, 2017

Any powers that you intend to actually remove need to be added to the dead.go files.

@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/openshift-jenkins/cmd e90c510 link /test cmd
ci/openshift-jenkins/integration e90c510 link /test integration
ci/openshift-jenkins/unit e90c510 link /test unit
ci/openshift-jenkins/extended_conformance_install e90c510 link /test extended_conformance_install

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.

@soltysh
Copy link
Contributor Author

soltysh commented Dec 6, 2017

This is superseded by #17549 and the remaining bits are just formatting to make it easier to compare with k8s bits.

@soltysh soltysh closed this Dec 6, 2017
@soltysh soltysh deleted the update_policy branch December 6, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants