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

add branch-allowlist repo-config parameter #1292

Closed
wants to merge 3 commits into from
Closed

add branch-allowlist repo-config parameter #1292

wants to merge 3 commits into from

Conversation

Steffen911
Copy link

@Steffen911 Steffen911 commented Dec 5, 2020

Context

Fixes #1028

What

This PR should add a branch_allowlist parameter to the RepoConfig that comes into effect in combination with the mergeable requirement.
In addition to checking if a PR is mergeable it also validates that it targets a set of allowed base branches.
Otherwise one might avoid the mergeable requirement by planning and applying on a non-protected branch.

Example

I set branch_allowlist: [master] and tried to apply a PR that is targeting the development branch. Error message:

image

Looking forward to all reviews :-)

@codecov
Copy link

codecov bot commented Dec 5, 2020

Codecov Report

Merging #1292 (84ba0d1) into master (eaafd43) will decrease coverage by 0.03%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1292      +/-   ##
==========================================
- Coverage   70.00%   69.97%   -0.04%     
==========================================
  Files          74       74              
  Lines        5585     5606      +21     
==========================================
+ Hits         3910     3923      +13     
- Misses       1312     1319       +7     
- Partials      363      364       +1     
Impacted Files Coverage Δ
server/events/models/models.go 78.99% <ø> (ø)
server/events/yaml/raw/global_cfg.go 0.00% <0.00%> (ø)
server/events/yaml/valid/repo_cfg.go 21.42% <ø> (ø)
server/events/yaml/valid/global_cfg.go 87.26% <60.00%> (-2.40%) ⬇️
server/events/project_command_runner.go 72.97% <66.66%> (-0.37%) ⬇️
server/events/project_command_builder.go 79.01% <100.00%> (+0.08%) ⬆️
server/events/yaml/raw/project.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaafd43...84ba0d1. Read the comment docs.

Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

👋 Thanks for contributing!

By writing e2e tests/running the server you should be able to figure out if there are other places to modify.

@nishkrishnan nishkrishnan added the waiting-on-response Waiting for a response from the user label Jan 28, 2021
@Steffen911 Steffen911 marked this pull request as draft January 30, 2021 10:58
@Steffen911 Steffen911 marked this pull request as ready for review January 30, 2021 13:14
@Steffen911
Copy link
Author

@nishkrishnan Thank you for your comment. I ran the commands locally with a test repository and confirmed that it works as expected now.

As soon as the code logic is reviewed I can also take care of the documentation, but I'd prefer to split the PRs to keep their size manageable.

@pinkavaj
Copy link

pinkavaj commented Feb 6, 2021

This one might be in conflict with #1383

The mentioned MR looks a little bit more generic and also clear and simpler for me.

@nishkrishnan nishkrishnan added waiting-on-review Waiting for a review from a maintainer and removed waiting-on-response Waiting for a response from the user labels Feb 8, 2021
@nishkrishnan
Copy link
Contributor

This one might be in conflict with #1383

The mentioned MR looks a little bit more generic and also clear and simpler for me.

I agree.

@nishkrishnan
Copy link
Contributor

Closing this in favor of: #1383

@nishkrishnan nishkrishnan added duplicate This issue is a duplicate of an already reported issue and removed waiting-on-review Waiting for a review from a maintainer labels Feb 25, 2021
@Steffen911 Steffen911 deleted the feat/1028-branch-allowlist branch February 25, 2021 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue is a duplicate of an already reported issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add branch allowlist capability
3 participants