-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add options for the pants-init action #21
Conversation
This should ensure that warnings aren't produced about $HOME/bin not being on the path.
Instead of hardcoding /tmp as the temporary path
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. lgtm overall bar the typos ;)
Thanks for catching the typos! Co-authored-by: Andreas Stenius <git@astekk.se>
Thanks for catching the typos @kaos! I just commited your suggestions, so should be all good now. |
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.
lgtm.
I'd prefer a second pair of eyes though, as I'm not that familiar with GH actions.
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 you've manually tested this, and are sure that the GH_TOKEN change is good?
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.
This is great. Making the cache paths configurable is superb, and cleaning up the if:
statements and the use of runner.temp
is excellent.
I'm concerned about injecting GH_TOKEN
into GITHUB_ENV
however, and I've left a comment about that.
Also I had a couple other minor questions.
@cognifloyd Thanks for the review! I confirmed the changes worked with the |
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.
That looks better. Thanks for testing my suggestion -- I'm glad it worked!
This adds options for:
It also does a little cleanup on some of the other steps. @jsirois encouraged me to submit this after this conversation on Slack