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

ConditionalApplyPlugin - allow apply on specific branches #25

Closed
kmanning opened this issue Sep 3, 2019 · 5 comments · Fixed by #325
Closed

ConditionalApplyPlugin - allow apply on specific branches #25

kmanning opened this issue Sep 3, 2019 · 5 comments · Fixed by #325
Milestone

Comments

@kmanning
Copy link
Collaborator

kmanning commented Sep 3, 2019

@kemper
Copy link
Contributor

kemper commented Jan 8, 2021

For allowing this plugin to be disabled, I take that to mean always apply? I can update my changes to support that. One option is to take an empty list of branches to mean "all"

ConditionalApplyPlugin.withBranchApplyEnabledFor([])

or an extra option:

ConditionalApplyPlugin.withBranchApplyEnabledFor([], all: true)

or a wildcard:

ConditionalApplyPlugin.withBranchApplyEnabledFor(['*'])

or I can add a new method and new field for controlling enable/disable.

ConditionalApplyPlugin.setEnabled(false)

@kmanning
Copy link
Collaborator Author

kmanning commented Jan 8, 2021

Let's split this issue into 2 - they are distinct. I updated the original post, and lets use this issue just for changing the branch from master to something else.

@kmanning kmanning added this to the v5.14 milestone Jan 8, 2021
@kmanning kmanning changed the title ConditionalApplyPlugin - allow customization ConditionalApplyPlugin - allow apply on specific branches Jan 11, 2021
@kmanning
Copy link
Collaborator Author

kmanning commented Jan 11, 2021

Issues #25 and #172 are related. I wonder if the code examples should be changed to reflect the relationship. Consider the following:

Issue #25 - allow apply on specific branches

ConditionalApplyPlugin.withApplyOnBranch('master')
// or alternatively
ConditionalApplyPlugin.withApplyOnBranch('master', 'test-branch')

Issue #172 - allow apply on specific branches

ConditionalApplyPlugin.withApplyOnEnvironment('qa')
// or alternatively
ConditionalApplyPlugin.withApplyOnEnvironment('qa', 'test-env')

@kemper
Copy link
Contributor

kemper commented Jan 13, 2021

Sounds good to me. Current the name/parameter combination on the PR is withBranchApplyEnabledFor(['branch1', 'branch2') . Do you want me to rename and change the params to be varargs?

@kmanning
Copy link
Collaborator Author

Yes on the rename.

On the varargs, it feels friendlier. So yes, I'd personally appreciate it, unless there's a good reason not to.

@kmanning kmanning linked a pull request Jan 19, 2021 that will close this issue
kmanning added a commit to kmanning/terraform-pipeline that referenced this issue Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants