Skip to content

Add issue approver output and enhance approval logic#206

Open
felpasl wants to merge 1 commit intotrstringer:mainfrom
felpasl:feat/190-add-approver-output
Open

Add issue approver output and enhance approval logic#206
felpasl wants to merge 1 commit intotrstringer:mainfrom
felpasl:feat/190-add-approver-output

Conversation

@felpasl
Copy link
Copy Markdown

@felpasl felpasl commented Jan 26, 2026

  • Updated action outputs to include the GitHub username of the approver.
  • Modified approval logic to return the approver's username upon approval or denial.
  • Enhanced tests to validate the new approver output.

related to #190

Comment thread action.yaml Outdated
- Updated action outputs to include the GitHub username of the approver.
- Modified approval logic to return the approver's username upon approval or denial.
- Enhanced tests to validate the new approver output.

Rename 'issue-approver' to 'issue-responder' for clarity in approval process
@felpasl felpasl force-pushed the feat/190-add-approver-output branch from e4012c2 to bac46d0 Compare January 27, 2026 11:09
@felpasl
Copy link
Copy Markdown
Author

felpasl commented Jan 30, 2026

@lizziemac If possible, please review these changes; otherwise, I may publish the fork for my own use.

thanks.

@snskArora
Copy link
Copy Markdown
Collaborator

@lizziemac If possible, please review these changes; otherwise, I may publish the fork for my own use.

thanks.

Hi @felpasl

Thanks a lot for this contribution. I understand that it has been very long, and you might have deployed your fork. The code changes look good to me. For good measure, is it possible to share some test results to show no breaking change for users operating with the previous version of inputs, and one test with your change?

Comment thread Makefile
test:
go test -v .

.PHONY: test_docker
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.

What is the purpose of this test_docker make target?

@lizziemac lizziemac added the Changes Requested Awaiting for a response on the open commnets label Apr 13, 2026
Comment thread Makefile
@@ -1,5 +1,6 @@
IMAGE_REPO=ghcr.io/trstringer/manual-approval
TARGET_PLATFORM=linux/amd64,linux/arm64,linux/arm/v8
GO_DOCKER_IMAGE=golang:1.24
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.

let's make a separate PR for this - ideally this image should match what is built & deployed

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.

and I'd like to add this to the readme, potentially even replace the current 'test' command

@lizziemac
Copy link
Copy Markdown
Collaborator

@felpasl Apologies for how long the back and forth is here, I wasn't getting emails for direct @'s. I can approve this once the test_docker target is separated out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changes Requested Awaiting for a response on the open commnets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants