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

#76 Adds support for mute args #77

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

Conversation

drewbrokke
Copy link

Hi @ChrisCarini, this is my first attempt at it.

For now, the input expects a comma-separated list of items, same as the CLI tool. I noticed that some of the options accept newlines or files as inputs, do you want to copy that pattern here instead? I didn't think there would be too much gained by following that pattern in this case since it's a small set of possible values. Thoughts?

ref: #76

Copy link
Owner

@ChrisCarini ChrisCarini left a comment

Choose a reason for hiding this comment

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

Generally LGTM! 🎉

@drewbrokke - Do you mind testing this action in a repo that I am able to view the action run output (and, sharing the link as part of this PR)? You should be able to test this via updating the action in a workflow to point to your fork at the respective commit hash and then kick off CI.

For now, the input expects a comma-separated list of items, same as the CLI tool. I noticed that some of the options accept newlines or files as inputs, do you want to copy that pattern here instead?

I think what you have now (if I'm understanding the usage/intent correctly) is fine. The action can grow later, if needed, in these ways.

I didn't think there would be too much gained by following that pattern in this case since it's a small set of possible values. Thoughts?

I agree - the newlines / 'files-as-inputs' came from past feature requests, I believe; I feel the same can happen here down the road, if needed.

entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Owner

@ChrisCarini ChrisCarini left a comment

Choose a reason for hiding this comment

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

It's also worth updating the README.md file with details about the new setting, too.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 1, 2024
@drewbrokke
Copy link
Author

@ChrisCarini I just force-pushed a couple of the suggested changes. Let me know if you'd like to see anything else adjusted. I will also post a link to a usage as soon as I have one. Thanks!

@ChrisCarini
Copy link
Owner

@ChrisCarini I just force-pushed a couple of the suggested changes. Let me know if you'd like to see anything else adjusted. I will also post a link to a usage as soon as I have one. Thanks!

LGTM! Thank you for incorporating the suggestions/changes! I'll try to give this a spin later this evening. Beyond that and pending your testing of the change, it looks ready to merge!

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

Successfully merging this pull request may close these issues.

None yet

2 participants