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 option to opt-out from pull request completion #135

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jsandquist
Copy link
Contributor

@jsandquist jsandquist commented Feb 21, 2023

Fix Azure/AzOps#741

Introducing a new variable AZOPS_SKIP_PR_COMPLETION AZOPS_COMPLETE_PULL_REQUEST that can be set to true false to opt-out from pull request completion should your workflow require mandatory pull request reviews.

Also converted two more steps to PowerShell following the work done in #120.

@jsandquist
Copy link
Contributor Author

Need to test this some more - it's a start for possible feedback. A bit unsure about the behaviour in case things go wrong as handled by > /dev/null 2>&1 in bash.

Write-Host "Pull Request created with Id: $prId"

# If PR is not completed, then optionally complete it bypassing policy
$completePullRequest = $env:AZOPS_COMPLETE_PULL_REQUEST -eq 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably do the opposite (compare with 'false') to keep consistent with old behaviour in case the variable is not set. And also consistent with the description in vars.yml.

Copy link
Contributor

Choose a reason for hiding this comment

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

We like consistency, so please change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the variable to AZOPS_SKIP_PR_COMPLETION in an attempt to better communicate intent as opposed to default behaviour.

.pipelines/pull.yml Outdated Show resolved Hide resolved
@jsandquist
Copy link
Contributor Author

jsandquist commented Mar 6, 2023

Need to test this some more - it's a start for possible feedback. A bit unsure about the behaviour in case things go wrong as handled by > /dev/null 2>&1 in bash.

From my initial tests I suppose this output is what would be hidden via the previous redirect but I need to verify this:

AzOps-Accelerator-135

The test cases included no variable (default) as well as switching the value between runs and with/without an existing 'automated' branch.

@daltondhcp
Copy link
Contributor

@SimonWahlin did you have the chance to look at this one?

@SimonWahlin
Copy link
Collaborator

@daltondhcp I did, but I see now that my review is still in pending state :(

I had some comments on using --query on the az commands but that is now solved with ConvertTo-Json which is absolutely fine.

Otherwise I think it looks good, but I have not had the opportunity to do any testing.

We could consider trying to do a stream redirect of the error message from AZ CLI and exit more gracefully if we fail to complete the PR, but it's a bit of an edge case so I would be happy with the "PowerShell exited with code '1'" error message.

@jsandquist
Copy link
Contributor Author

jsandquist commented Apr 5, 2023

I have some very minor adjustments on its way. I've done some basic testing for the default pipeline here but on a daily basis I use a variation of this with the pull pipeline split into two jobs instead of a single one.

Also, as far as I understand this is not needed with GitHub Actions, right?

Finally - here is a sample edge case - could possibly output some info on what to do next, but that might be for the wiki instead

image

@SimonWahlin
Copy link
Collaborator

@jsandquist Ping me when you are satisfied and I'll do some tests on my side as well

@jsandquist
Copy link
Contributor Author

@SimonWahlin Finally updated it so ready for some tests on your side now hopefully. I refrained from splitting the pull pipeline into two jobs named 'pull' and 'merge' respectively - that might be another Pull Request at some point but complicates the setup somewhat.

@SimonWahlin
Copy link
Collaborator

Finally sat down and tested this (sorry for the delay).

It still sets the PR to auto-complete and will have no effect unless there is a branch policy requiring for example reviews. Maybe we should consider naming the setting AZOPS_SKIP_PR_POLICYBYPASS, or at least mention that in the comment in vars.yml.

Other than that, it works as expected, setting the variable to true will not bypass branch policy, setting it to anything else than true or not setting it at all will still bypass any policies.

Copy link
Collaborator

@SimonWahlin SimonWahlin left a comment

Choose a reason for hiding this comment

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

Looks good, but @daltondhcp has to decide what we should call the settings variable per my earlier comment

.pipelines/pull.yml Outdated Show resolved Hide resolved
.pipelines/pull.yml Outdated Show resolved Hide resolved
@jsandquist
Copy link
Contributor Author

AZOPS_SKIP_PR_POLICYBYPASS

Good proposal. I'll adjust the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants