-
Notifications
You must be signed in to change notification settings - Fork 101
add repo's re-usable action #207
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
Conversation
|
Thanks for proposing this! As mentioned in #205 I think this should live in its own GitHub repo. We can then add a reference to the nbstripout README for how to use it. This means you can maintain the action yourself and are not dependent on me for making changes. What do you think? |
|
I wouldn't create a separate repo only for adding this re-usable action, as I am not adding any core functionality in |
|
How did you test this? And how would I be able to test this? If you really think action should be in the main nbstripout repo it definitely needs extensive documentation in the README :) |
|
I found out this interesting tool for testing on-development actions: nektos |
| on: | ||
| pull_request: | ||
| paths: | ||
| - '**.ipynb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this doesn't recurse into subdirectories? Same for push below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what does this rule sets, is the actual event trigger rule for the workflow.
**.ipynb means that if any .ipynb files are included into a PR or a pushed commit to main branch then the guard check will be triggered. These section is independent of the re-usable action and its up to users to define their own trigger rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this match .ipynb files in subdirectories also? Or only the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The **.ipynb will match any files of type .ipynb in subdirectories recursively, if someone wants to match the event trigger for a specific subdir, they can do something like <subdir>/*.ipynb or recursively under a subdir like:
<subdir>/**/*.ipynb
| - main | ||
| paths: | ||
| - '**.ipynb' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be empty? Is this valid syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, workflow_dispatch just enables custom triggering of a workflow.
action.yml
Outdated
|
|
||
| if [ -z "$notebooks" ]; then | ||
| echo "No notebook files found" | ||
| echo "notebooks=" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for passing output to the action below? The documentation mentions some other requirements. Is this not needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax echo "message" >> $GITHUB_OUTPUT
will propagate the to the workflow that will use the nbstripout action.
Here we can just remove the echoing because "notebooks=" is empty when no notebooks found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra arguments you are refering to in the docs are for passing the output between dependant workflows, but we don't have such an advanced case here.
kynan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Let's submit this and iterate as needed.
Implementing repos action #205
Then users can leverage it in a simple workflow like: