-
Notifications
You must be signed in to change notification settings - Fork 127
Add config option to specify branches for dangerous workflow #677
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Samuel Erb <[email protected]>
It's not really clear why the linter is failing, I can't replicate this locally with:
|
Signed-off-by: Samuel Erb <[email protected]>
Looks like 24b42f0 resolved the lint error. |
pkg/config/config.go
Outdated
@@ -65,6 +65,11 @@ type OrgConfig struct { | |||
|
|||
// Schedule specifies whether to perform certain actions on specific days. | |||
Schedule *ScheduleConfig `json:"schedule"` | |||
|
|||
// Comma-seperated branch list to scan for Dangerous Workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: separated. Same for below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
Signed-off-by: Samuel Erb <[email protected]>
What happens if a given branch isn't available in a repo? Will it just skip or throw an error? I'd want to specify "refs/remotes/origin/master", "refs/remotes/origin/main" etc. to catch different default branches in various repos. Or is there a way to always pick the default branch? |
Based on https://github.com/serb-google/allstar/blob/config/pkg/policies/workflow/workflow.go#L150 this should throw an error. I think it should be possible to wire up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some naming comments.
Lets preserve the ability to easily only check the default branch, as @coheigea mentioned.
// Comma-separated branch list to scan for Dangerous Workflows. | ||
// Blank/default to scan all branches. | ||
// Must use format "refs/remotes/origin/branch_name". | ||
DangerousWorkflowBranchList string `json:"dangerousWorkflowBranchList"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you need these config fields. Just the ones you added to the policy-specific config
// Comma-separated branch list to scan for Dangerous Workflows. | ||
// Blank/default to scan all branches. | ||
// Must use format "refs/remotes/origin/branch_name". | ||
DangerousWorkflowBranchList string `json:"dangerousWorkflowBranchList"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is already a part of the Dangerous Workflow policy config, just name this "BranchList"
Add config option to specify branches for dangerous workflow, see #622 (comment)
Tested with and without option in
/.allstar/dangerous_workflow.yaml
file private branch to confirm this works as expected.