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 a branch matcher to the server side repo config #1383

Merged
merged 1 commit into from
Feb 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions runatlantis.io/docs/server-side-repo-config.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ repos:
# Repo ID's are of the form {VCS hostname}/{org}/{repo name}, ex.
# github.com/runatlantis/atlantis.
- id: /.*/
# branch is an regex matching pull requests by base branch
# (the branch the pull request is getting merged into).
# By default, all branches are matched
branch: /.*/

# apply_requirements sets the Apply Requirements for all repos that match.
apply_requirements: [approved, mergeable]
Expand Down
2 changes: 1 addition & 1 deletion server/events/pre_workflow_hooks_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(

preWorkflowHooks := make([]*valid.PreWorkflowHook, 0)
for _, repo := range w.GlobalCfg.Repos {
if repo.IDMatches(baseRepo.ID()) && len(repo.PreWorkflowHooks) > 0 {
if repo.IDMatches(baseRepo.ID()) && repo.BranchMatches(pull.BaseBranch) && len(repo.PreWorkflowHooks) > 0 {
preWorkflowHooks = append(preWorkflowHooks, repo.PreWorkflowHooks...)
}
}
Expand Down
15 changes: 14 additions & 1 deletion server/events/yaml/parser_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1014,11 +1014,17 @@ func TestParseGlobalCfg(t *testing.T) {
- apply_requirements: []`,
expErr: "repos: (0: (id: cannot be blank.).).",
},
"invalid regex": {
"invalid id regex": {
input: `repos:
- id: /?/`,
expErr: "repos: (0: (id: parsing: /?/: error parsing regexp: missing argument to repetition operator: `?`.).).",
},
"invalid branch regex": {
input: `repos:
- id: /.*/
branch: /?/`,
expErr: "repos: (0: (branch: parsing: /?/: error parsing regexp: missing argument to repetition operator: `?`.).).",
},
"workflow doesn't exist": {
input: `repos:
- id: /.*/
Expand Down Expand Up @@ -1115,6 +1121,7 @@ repos:
allowed_overrides: [apply_requirements, workflow]
allow_custom_workflows: true
- id: /.*/
branch: /(master|main)/
pre_workflow_hooks:
- run: custom workflow command
workflows:
Expand Down Expand Up @@ -1155,6 +1162,7 @@ policies:
},
{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile("(master|main)"),
PreWorkflowHooks: preWorkflowHooks,
},
},
Expand Down Expand Up @@ -1228,6 +1236,7 @@ workflows:
Repos: []valid.Repo{
{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile(".*"),
PreWorkflowHooks: []*valid.PreWorkflowHook{},
ApplyRequirements: []string{},
Workflow: &valid.Workflow{
Expand Down Expand Up @@ -1301,6 +1310,10 @@ workflows:
Assert(t, expRepo.IDRegex.String() == actRepo.IDRegex.String(),
"%q != %q for repos[%d]", expRepo.IDRegex.String(), actRepo.IDRegex.String(), i)
}
if expRepo.BranchRegex != nil {
Assert(t, expRepo.BranchRegex.String() == actRepo.BranchRegex.String(),
"%q != %q for repos[%d]", expRepo.BranchRegex.String(), actRepo.BranchRegex.String(), i)
}
}
})
}
Expand Down
24 changes: 24 additions & 0 deletions server/events/yaml/raw/global_cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type GlobalCfg struct {
// Repo is the raw schema for repos in the server-side repo config.
type Repo struct {
ID string `yaml:"id" json:"id"`
Branch string `yaml:"branch" json:"branch"`
ApplyRequirements []string `yaml:"apply_requirements" json:"apply_requirements"`
PreWorkflowHooks []PreWorkflowHook `yaml:"pre_workflow_hooks" json:"pre_workflow_hooks"`
Workflow *string `yaml:"workflow,omitempty" json:"workflow,omitempty"`
Expand Down Expand Up @@ -121,6 +122,11 @@ func (r Repo) HasRegexID() bool {
return strings.HasPrefix(r.ID, "/") && strings.HasSuffix(r.ID, "/")
}

// HasRegexBranch returns true if a branch regex was set.
func (r Repo) HasRegexBranch() bool {
return strings.HasPrefix(r.Branch, "/") && strings.HasSuffix(r.Branch, "/")
}

func (r Repo) Validate() error {
idValid := func(value interface{}) error {
id := value.(string)
Expand All @@ -131,6 +137,15 @@ func (r Repo) Validate() error {
return errors.Wrapf(err, "parsing: %s", id)
}

branchValid := func(value interface{}) error {
branch := value.(string)
if !r.HasRegexBranch() {
return nil
}
_, err := regexp.Compile(branch[1 : len(branch)-1])
return errors.Wrapf(err, "parsing: %s", branch)
}

overridesValid := func(value interface{}) error {
overrides := value.([]string)
for _, o := range overrides {
Expand All @@ -149,6 +164,7 @@ func (r Repo) Validate() error {

return validation.ValidateStruct(&r,
validation.Field(&r.ID, validation.Required, validation.By(idValid)),
validation.Field(&r.Branch, validation.By(branchValid)),
validation.Field(&r.AllowedOverrides, validation.By(overridesValid)),
validation.Field(&r.ApplyRequirements, validation.By(validApplyReq)),
validation.Field(&r.Workflow, validation.By(workflowExists)),
Expand All @@ -166,6 +182,13 @@ func (r Repo) ToValid(workflows map[string]valid.Workflow) valid.Repo {
id = r.ID
}

var branchRegex *regexp.Regexp
if r.HasRegexBranch() {
withoutSlashes := r.Branch[1 : len(r.Branch)-1]
// Safe to use MustCompile because we test it in Validate().
branchRegex = regexp.MustCompile(withoutSlashes)
}
Copy link

Choose a reason for hiding this comment

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

Consider allowing specify single branch using plain branch-name syntax, similar to non-regex for the id.

Copy link
Contributor Author

@dghubble dghubble Feb 6, 2021

Choose a reason for hiding this comment

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

I considered this. id allows a string or regex, but it feels kinda cumbersome. The default value needs to continue matching all branches. I favored branch field always being a regex (default .*) (and if you want to match a single name, you could just set it to ^master$) so we don't need to condition on which sort of string it is.

If a maintainer prefers the opposite, I'm happy to refactor.

Copy link

Choose a reason for hiding this comment

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

ok


var workflow *valid.Workflow
if r.Workflow != nil {
// This key is guaranteed to exist because we test for it in
Expand All @@ -184,6 +207,7 @@ func (r Repo) ToValid(workflows map[string]valid.Workflow) valid.Repo {
return valid.Repo{
ID: id,
IDRegex: idRegex,
BranchRegex: branchRegex,
ApplyRequirements: r.ApplyRequirements,
PreWorkflowHooks: preWorkflowHooks,
Workflow: workflow,
Expand Down
10 changes: 10 additions & 0 deletions server/events/yaml/valid/global_cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Repo struct {
// IDRegex is the regex match for this config.
// If ID is set then this will be nil.
IDRegex *regexp.Regexp
BranchRegex *regexp.Regexp
ApplyRequirements []string
PreWorkflowHooks []*PreWorkflowHook
Workflow *Workflow
Expand Down Expand Up @@ -123,6 +124,7 @@ func NewGlobalCfgWithHooks(allowRepoCfg bool, mergeableReq bool, approvedReq boo
Repos: []Repo{
{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile(".*"),
ApplyRequirements: applyReqs,
PreWorkflowHooks: preWorkflowHooks,
Workflow: &defaultWorkflow,
Expand Down Expand Up @@ -158,6 +160,14 @@ func (r Repo) IDMatches(otherID string) bool {
return r.IDRegex.MatchString(otherID)
}

// BranchMatches returns true if the branch other matches a branch regex (if preset).
func (r Repo) BranchMatches(other string) bool {
if r.BranchRegex == nil {
return true
}
return r.BranchRegex.MatchString(other)
}

// IDString returns a string representation of this config.
func (r Repo) IDString() string {
if r.ID != "" {
Expand Down
21 changes: 21 additions & 0 deletions server/events/yaml/valid/global_cfg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ func TestNewGlobalCfg(t *testing.T) {
Repos: []valid.Repo{
{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile(".*"),
ApplyRequirements: []string{},
Workflow: &expDefaultWorkflow,
AllowedWorkflows: []string{},
Expand Down Expand Up @@ -108,6 +109,7 @@ func TestNewGlobalCfg(t *testing.T) {
// For each test, we change our expected cfg based on the parameters.
exp := deepcopy.Copy(baseCfg).(valid.GlobalCfg)
exp.Repos[0].IDRegex = regexp.MustCompile(".*") // deepcopy doesn't copy the regex.
exp.Repos[0].BranchRegex = regexp.MustCompile(".*")

if c.allowRepoCfg {
exp.Repos[0].AllowCustomWorkflows = Bool(true)
Expand All @@ -132,6 +134,10 @@ func TestNewGlobalCfg(t *testing.T) {
Assert(t, expRepo.IDRegex.String() == actRepo.IDRegex.String(),
"%q != %q for repos[%d]", expRepo.IDRegex.String(), actRepo.IDRegex.String(), i)
}
if expRepo.BranchRegex != nil {
Assert(t, expRepo.BranchRegex.String() == actRepo.BranchRegex.String(),
"%q != %q for repos[%d]", expRepo.BranchRegex.String(), actRepo.BranchRegex.String(), i)
}
}
})
}
Expand Down Expand Up @@ -726,6 +732,21 @@ func TestRepo_IDString(t *testing.T) {
Equals(t, "/regex.*/", (valid.Repo{IDRegex: regexp.MustCompile("regex.*")}).IDString())
}

func TestRepo_BranchMatches(t *testing.T) {
// Test matches when no branch regex is set.
Equals(t, true, (valid.Repo{}).BranchMatches("main"))

// Test regexes.
Equals(t, true, (valid.Repo{BranchRegex: regexp.MustCompile(".*")}).BranchMatches("main"))
Equals(t, true, (valid.Repo{BranchRegex: regexp.MustCompile("main")}).BranchMatches("main"))
dghubble marked this conversation as resolved.
Show resolved Hide resolved
Equals(t, false, (valid.Repo{BranchRegex: regexp.MustCompile("^main$")}).BranchMatches("foo-main"))
Equals(t, false, (valid.Repo{BranchRegex: regexp.MustCompile("^main$")}).BranchMatches("main-foo"))
Equals(t, true, (valid.Repo{BranchRegex: regexp.MustCompile("(main|master)")}).BranchMatches("main"))
Equals(t, true, (valid.Repo{BranchRegex: regexp.MustCompile("(main|master)")}).BranchMatches("master"))
Equals(t, true, (valid.Repo{BranchRegex: regexp.MustCompile("release")}).BranchMatches("release-stage"))
Equals(t, false, (valid.Repo{BranchRegex: regexp.MustCompile("release")}).BranchMatches("main"))
}

// String is a helper routine that allocates a new string value
// to store v and returns a pointer to it.
func String(v string) *string { return &v }
Expand Down