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

feat: ✨ pass additional pathspec with action arguments #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akaguny
Copy link

@akaguny akaguny commented Nov 25, 2023

ref: #7

Testing:

  1. create 2 files with fixme text (Readme.md and file-with-fixme-that-throw.md) https://github.com/akaguny/test-action-fixme-check
  2. exclude README.md (.github excludes by default) https://github.com/akaguny/test-action-fixme-check/blob/main/.github/workflows/test.yaml#L13

expected:
only file-with-fixme-that-throw will throw Error
actual:
👌only file-with-fixme-that-throw will throw Error
https://github.com/akaguny/test-action-fixme-check/actions/runs/6989397876/job/19017976882
image

notes (for next devs maybe =) ):

  • arguments available in bash script passed as $1, $2 order is matter and describe in action.yaml/runs/args

@bbugh
Copy link
Owner

bbugh commented Dec 5, 2023

Hey @akaguny! Thanks for the PR, I can see how this could be useful in some circumstances.

However, it looks like the implementation only allows a single pathspec, which constrains the value of it too much for me to accept as-is to main. If you update it to allow multiple space-separated values then that would be acceptable.

I believe the only change needed to support multiple path specs is to expand pathspec as an array instead of a quoted single variable:

- -- ":^.github" "${pathspec}")
+ -- ":^.github" ${pathspec[@]}

but you'll need to do a bit of testing to make sure that folders with spaces and strings with quotes makes it through, because shell escaping and quoting is so fraught with issues and yaml strings are so loosey-goosey.

Let me know if you have any questions.

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.

2 participants