SecurityContextConstraints: update description of the Priority field #15425

php-coder · 2017-07-24T12:17:53Z

Sync description with reality and documentation.

pweil-

LGTM. May want to quote "nil" so it is obvious it's a literal value. Otherwise it looks like it should be capitalized to me 😄

php-coder · 2017-07-24T12:36:33Z

@pweil- Thanks! I've added quotes.

simo5 · 2017-07-24T12:48:16Z

LGTM [test]

openshift-bot · 2017-07-24T12:56:00Z

Evaluated for origin test up to 56452ce

openshift-bot · 2017-07-24T15:10:37Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3441/) (Base Commit: 74121fa) (PR Branch Commit: 56452ce)

php-coder · 2017-07-25T14:29:19Z

Test flake #13536

php-coder · 2017-07-31T09:40:20Z

@mfojtik And this one too, please :)

mfojtik · 2017-07-31T09:49:06Z

pkg/security/apis/security/types.go

@@ -16,7 +16,9 @@ type SecurityContextConstraints struct {

 	// Priority influences the sort order of SCCs when evaluating which SCCs to try first for
 	// a given pod request based on access in the Users and Groups fields.  The higher the int, the
-	// higher priority.  If scores for multiple SCCs are equal they will be sorted by name.
+	// higher priority. "nil" is considered a 0 priority. If scores for multiple SCCs are equal they


i guess "nil" is not what you use when you want to set this via api (iow. you don't do "priority": "nil". I would use the word "not set" instead of "nil".

I've updated it to "An unset value is considered a 0 priority." PTAL

mfojtik · 2017-07-31T09:49:25Z

@php-coder one comment, /approved

php-coder · 2017-08-03T14:51:14Z

It failed because of #15579

php-coder · 2017-08-04T10:21:38Z

/test extended_conformance_install_update

php-coder · 2017-08-04T12:55:04Z

/retest

php-coder · 2017-08-04T12:58:41Z

@stevekuznetsov @deads2k This PR modifies API-related files and hence has "needs-api-review" tag but it changes only a comment. What is a procedure for merging such PRs?

stevekuznetsov · 2017-08-04T13:07:45Z

@php-coder you should probably get a review from someone in @openshift/sig-security since the comments are exposed to end-users

php-coder · 2017-08-07T14:08:18Z

/test extended_templates

php-coder · 2017-08-07T14:09:02Z

@openshift/security PTAL

simo5 · 2017-08-07T16:48:50Z

/lgtm

pweil- · 2017-08-07T17:04:51Z

/approved

stevekuznetsov · 2017-08-07T18:39:59Z

@Kargakis looks like we need to honor the api-approved label -- don't add the needs-api-review if we have it

0xmichalis · 2017-08-07T22:08:06Z

@Kargakis looks like we need to honor the api-approved label -- don't add the needs-api-review if we have it

@deads2k already asked for it; we need to move path-label in a prow plugin and make it configurable so that it can choose from a whitelist what labels to add either once or always. Not a high priority.

0xmichalis · 2017-08-07T22:09:03Z

Meaning we need to sort out cherry-picks and flake detection first.

php-coder · 2017-08-08T10:47:19Z

@stevekuznetsov @Kargakis Could someone then merge it manually?

stevekuznetsov · 2017-08-08T13:35:21Z

@php-coder it will merge fine even with the label

pweil- · 2017-08-08T13:36:48Z

/approve

php-coder · 2017-08-09T17:14:33Z

@php-coder it will merge fine even with the label

@stevekuznetsov ... but when?

php-coder · 2017-08-09T17:16:50Z

What if I don't want to wait when @mfojtik came back from his PTO to merge a trivial PR that updates only comments? Is it possible with our infrastructure?

pweil- · 2017-08-09T18:31:33Z

@php-coder as soon as my name gets added to owners #15712

0xmichalis · 2017-08-09T19:45:58Z

@stevekuznetsov ... but when?

You miss an approved label. It should be possible to determine it by looking at the "Submit Queue" github status context below ("PR does not have approved label. "). You can also click at the Details button, under the MERGE REQUIREMENTS section you can read:

PRs must meet the following set of conditions to be considered for automatic merging by the submit queue.

    The PR must be mergeable. aka cannot need a rebase
    All of the following github statuses must be green
        ci/openshift-jenkins/verify
        ci/openshift-jenkins/unit
        ci/openshift-jenkins/cmd
        ci/openshift-jenkins/integration
        ci/openshift-jenkins/end_to_end
        ci/openshift-jenkins/extended_conformance_install_update
        ci/openshift-jenkins/extended_conformance_gce
        ci/openshift-jenkins/extended_networking_minimal
        ci/openshift-jenkins/extended_templates
    The PR cannot have any of the following milestones: []
    The PR must have the "lgtm" label
    The PR must not have been updated since the "lgtm" label was applied
    The PR must have the "approved" label
    The PR must not have the "do-not-merge" label


The PR can then be queued to re-test before merge. Once it reaches the top of the queue all of the above conditions must be true but so must the following:

    All of the following tests must pass a second time
        ci/openshift-jenkins/verify
        ci/openshift-jenkins/unit
        ci/openshift-jenkins/cmd
        ci/openshift-jenkins/integration
        ci/openshift-jenkins/end_to_end
        ci/openshift-jenkins/extended_conformance_install_update
        ci/openshift-jenkins/extended_conformance_gce
        ci/openshift-jenkins/extended_networking_minimal
        ci/openshift-jenkins/extended_templates
    Unless the "retest-not-required" or "retest-not-required-docs-only" label is present

And then the PR will be merged!!

Sorry if it may not be obvious enough. We should create a document that aggregates explanations for all of our new CI tools, I think @stevekuznetsov has made a start.

php-coder · 2017-08-09T20:09:48Z

@Kargakis The behavior that surprised me is that when @pweil- left /approve comment, the bot had done nothing. I'd expect that it should says that Paul doesn't have permissions to approve. AFAIR k8s bots follow this behavior.

0xmichalis · 2017-08-09T20:17:55Z

@Kargakis The behavior that surprised me is that when @pweil- left /approve comment, the bot had done nothing. I'd expect that it should says that Paul doesn't have permissions to approve. AFAIR k8s bots follow this behavior.

I see the bot added a comment: #15425 (comment)

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: php-coder, pweil-, simo5
We suggest the following additional approver: mfojtik

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

php-coder · 2017-08-09T20:22:35Z

I see the bot added a comment.

Hm. Thank you for teaching me. Probably, I didn't understand how it works before and thought that approval from one person is enough to have approved label. I see that the rules a bit more complex.

stevekuznetsov · 2017-08-10T17:10:27Z

You need an approval from one person from each of the directories that it touches. The bot says so in the comment:

Needs approval from an approver in each of these OWNERS Files:
  -  OWNERS

If we changed more than one directory, they would all be listed there. An approval from someone in a top-level dir will approve dirs underneath.

Since this has been LGTMed by @pweil- and @mfojtik I'll /approve

@php-coder please let us know if the UX is not optimal, but also please be sure to read the comments the bot writes. The extra complexity does give us benefits -- requiring LGTM and approval gives a small set of approvers ability to delegate nitty-gritty reviews to reviewers, requiring approval from all the dirs allows us to ensure a code change gets looked at by all the right people, etc.

stevekuznetsov · 2017-08-10T17:10:40Z

/approve

openshift-merge-robot · 2017-08-10T17:10:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: php-coder, pweil-, simo5, stevekuznetsov

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [pweil-,stevekuznetsov]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-08-10T21:03:57Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-10T23:10:10Z

Automatic merge from submit-queue

pweil- approved these changes Jul 24, 2017

View reviewed changes

php-coder force-pushed the scc_improve_priority_desc branch from 7ef958a to 56452ce Compare July 24, 2017 12:35

openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned pweil- and mfojtik Jul 25, 2017

openshift deleted a comment from openshift-merge-robot Jul 25, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

openshift deleted a comment from openshift-merge-robot Jul 31, 2017

mfojtik reviewed Jul 31, 2017

View reviewed changes

php-coder added 2 commits August 3, 2017 15:18

SecurityContextConstraints: update description of the Priority field.

dbea4af

Update generated files.

dc63045

php-coder force-pushed the scc_improve_priority_desc branch from 56452ce to dc63045 Compare August 3, 2017 13:18

openshift-merge-robot added the needs-api-review label Aug 4, 2017

openshift-ci-robot assigned simo5 Aug 7, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2017

pweil- added api-approved and removed needs-api-review labels Aug 7, 2017

openshift-merge-robot added the needs-api-review label Aug 7, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2017

openshift-merge-robot merged commit a19b41e into openshift:master Aug 10, 2017

php-coder deleted the scc_improve_priority_desc branch August 11, 2017 06:24

SecurityContextConstraints: update description of the Priority field #15425

SecurityContextConstraints: update description of the Priority field #15425

Conversation

php-coder commented Jul 24, 2017

pweil- left a comment

Choose a reason for hiding this comment

php-coder commented Jul 24, 2017

simo5 commented Jul 24, 2017

openshift-bot commented Jul 24, 2017

openshift-bot commented Jul 24, 2017

php-coder commented Jul 25, 2017

php-coder commented Jul 31, 2017

mfojtik Jul 31, 2017

Choose a reason for hiding this comment

php-coder Aug 3, 2017

Choose a reason for hiding this comment

mfojtik commented Jul 31, 2017

php-coder commented Aug 3, 2017

php-coder commented Aug 4, 2017

php-coder commented Aug 4, 2017

php-coder commented Aug 4, 2017

stevekuznetsov commented Aug 4, 2017

php-coder commented Aug 7, 2017

php-coder commented Aug 7, 2017

simo5 commented Aug 7, 2017

pweil- commented Aug 7, 2017

stevekuznetsov commented Aug 7, 2017

0xmichalis commented Aug 7, 2017

0xmichalis commented Aug 7, 2017

php-coder commented Aug 8, 2017

stevekuznetsov commented Aug 8, 2017

pweil- commented Aug 8, 2017

php-coder commented Aug 9, 2017

php-coder commented Aug 9, 2017

pweil- commented Aug 9, 2017

0xmichalis commented Aug 9, 2017 • edited Loading

php-coder commented Aug 9, 2017 • edited Loading

0xmichalis commented Aug 9, 2017 • edited Loading

php-coder commented Aug 9, 2017

stevekuznetsov commented Aug 10, 2017

stevekuznetsov commented Aug 10, 2017

openshift-merge-robot commented Aug 10, 2017

openshift-merge-robot commented Aug 10, 2017

openshift-merge-robot commented Aug 10, 2017

0xmichalis commented Aug 9, 2017 •

edited

Loading

php-coder commented Aug 9, 2017 •

edited

Loading

0xmichalis commented Aug 9, 2017 •

edited

Loading