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

Blockade prow plugin (formerly block-paths munger). #4944

Merged
merged 2 commits into from
Oct 12, 2017

Conversation

cjwagner
Copy link
Member

/cc @rmmh

@cjwagner cjwagner requested a review from 0xmichalis as a code owner October 10, 2017 01:22
@k8s-ci-robot k8s-ci-robot requested a review from rmmh October 10, 2017 01:22
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 10, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2017
@cjwagner cjwagner force-pushed the blockade branch 2 times, most recently from 3c0db6d to b840947 Compare October 10, 2017 01:49
- kubernetes-incubator
- kubernetes/kubernetes
- kubernetes/test-infra
blockregexps:
Copy link
Contributor

Choose a reason for hiding this comment

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

document the flow at the top: default allow, deny if a changed file matches any of blockregexps, but allow if it matches any of exception regexps. blocking means adding do-not-merge/blocked-paths.


const (
pluginName = "blockade"
blockedPathsLabel = "do-not-merge/blocked-paths"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be part of the config? Bad docs changes had a special label before. Other users might want a different label to be applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we let each blockade specify its own label and a blockade is removed, we won't know about that blockade's label anymore and won't be able to remove it from PRs that already have it.
We could allow partial customization of the label by allowing blockades to specify a label suffix and prefix all labels with do-not-merge/blockade/ or something like that. Then the plugin can remove any labels with the prefix that no longer apply. That would result in long label names and additional label bloat though.

explanation string
}

func (bd *blockade) isBlocker(file string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be isBlocked

labelPresent := hasBlockedLabel(labels)
blockades := compileApplicableBlockades(org, repo, c.log, config)
if len(blockades) == 0 && !labelPresent {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

// if the label is missing, we assume that we removed any associated comments.

*/

/*
Example config for this plugin:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be above the actual config definition so that you get it with go doc.

@cjwagner
Copy link
Member Author

Comments addressed. PTAL

- kubernetes/kubernetes
- kubernetes/test-infra
blockregexps:
- "docs/.*"
Copy link
Contributor

Choose a reason for hiding this comment

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

use ' to quote regexps to avoid interpreting \ escapes

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that valid yaml? Back ticks don't appear to be a valid replacement for quotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

not back ticks, single quotes

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok. Fixed.

Besides migrating this commit makes the behavior configurable by repo, org, or set of org and
repos. Additionally it generates explanations of why each blocked PR was
blocked.
@rmmh
Copy link
Contributor

rmmh commented Oct 12, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, rmmh

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

@k8s-ci-robot k8s-ci-robot merged commit ddf1adc into kubernetes:master Oct 12, 2017
@cjwagner cjwagner deleted the blockade branch October 12, 2017 21:32
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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.

5 participants