Skip to content

feat(approval-flow): treat issue closure as denial#209

Open
snskArora wants to merge 3 commits intotrstringer:mainfrom
snskArora:feature/deny-on-issue-closure
Open

feat(approval-flow): treat issue closure as denial#209
snskArora wants to merge 3 commits intotrstringer:mainfrom
snskArora:feature/deny-on-issue-closure

Conversation

@snskArora
Copy link
Copy Markdown
Collaborator

This PR aims to resolve #199

Testing workflows:

  1. testing the existing workflow (backward compatibility, default behaviour)
    https://github.com/snskArora/sandbox/actions/runs/24009833832/job/70019520102
    Manual approval required for workflow run 24009833832 snskArora/sandbox#35
    Closing the issue doesnt have any effect if the variable close-issue-means-denial is missing

  2. testing issue closure denies the workflow or not
    close-issue-means-denial: true set in action inputs
    https://github.com/snskArora/sandbox/actions/runs/24009867780/job/70019646047
    Manual approval required for workflow run 24009867780 snskArora/sandbox#37

  3. testing fail-on-denial with this functionality
    fail-on-denial: false & close-issue-means-denial: true set in action inputs
    https://github.com/snskArora/sandbox/actions/runs/24009899588/job/70019720817
    Manual approval required for workflow run 24009899588 snskArora/sandbox#39

Copy link
Copy Markdown
Collaborator

@lizziemac lizziemac 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 to me - had one thought but I'm fine with you merging if you don't think it's necessary (it's been a while since I've been in this repo and your knowledge is fresher)

Comment thread main.go
close(channel)
case approvalStatusPending:
if apprv.closeIssueMeansDenial {
issue, _, err := client.Issues.Get(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should only call this every X loops to reduce API calls

granted it's only when closeIssueMeansDenial is set, but worth considering

Comment thread main.go
fmt.Printf("error commenting on closed issue: %v\n", err)
}
channel <- 1
close(channel)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

add return here, per this PR's bug fix (#210)

Suggested change
close(channel)
close(channel)
return

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 this pull request may close these issues.

Treat issue closure as denial

2 participants