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

Bundle limitations and gotchas together in the README.md #73

Merged
merged 8 commits into from May 10, 2020
Merged

Bundle limitations and gotchas together in the README.md #73

merged 8 commits into from May 10, 2020

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented May 5, 2020

The documentation of this action is really good and has a lot of information. We can make it easier to use by putting a common example first, then clearly listing the limitations and possible gotchas.

I found that there was a bit of duplication, so i attempted to consolidate and distill it down to the essentials.

Thank you for this package!

@spawnia
Copy link
Contributor Author

spawnia commented May 5, 2020

Is there any downside to always defining ${{ github.head_ref }}? I think we might save even more people the trouble if we just say it's required.

@stefanzweifel
Copy link
Owner

Thanks for this PR! 🙌

Is there any downside to always defining ${{ github.head_ref }}? I think we might save even more people the trouble if we just say it's required.

There is. github.head_ref is only available when the Action is used using the pull_request event. That's why we can't use it as a default value.

@spawnia
Copy link
Contributor Author

spawnia commented May 6, 2020

There is. github.head_ref is only available when the Action is used using the pull_request event. That's why we can't use it as a default value.

It does not seem to error when trying to get its value, so the result is that you just get an empty string - which is what the current default is, if i am not mistaken.

I actually just tried running it directly on a branch with ${{ github.head_ref}} and it worked perfectly: https://github.com/spawnia/sailor/runs/650680073

@stefanzweifel
Copy link
Owner

Hm, seems this has changed since I tested this the last time. 😅

If this actually works, I think it would even make sense to use github.head_ref as the default value for branch. (Will do some testing in the next days)

@stefanzweifel
Copy link
Owner

Using github.head_ref as a default value actually works. See #75
I've tested this in a test project while listening to push and pull_request-events.

One downside: For pull_request-events the checkouts-Action still has to be updated with a ref-input. Otherwise the repo would be in a detached state.

Still thinking about if this would be a good approach. Would merge those PRs and update the README with an updated example, if I think we should go down that path.

@spawnia
Copy link
Contributor Author

spawnia commented May 10, 2020

@stefanzweifel i simplified the README further and clarified the branch option is only needed to push to other branches. Not sure if that use case works, i have not tried it.

@stefanzweifel
Copy link
Owner

Just wanted to update the README too. Will do some last testing and then tag 4.2.0

@spawnia
Copy link
Contributor Author

spawnia commented May 10, 2020

@stefanzweifel feel free to edit my branch as needed or pick the parts you like :)

@stefanzweifel stefanzweifel merged commit 02c4f2c into stefanzweifel:master May 10, 2020
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.

None yet

2 participants