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

docs: Clarify usage of event triggers in README #58

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

bytestream
Copy link
Contributor

Is the example config currently correct?

I couldn't get it to work... the only way I got it to work was after looking at https://github.com/amannn/action-semantic-pull-request/blob/master/.github/workflows/lint-pr-title.yml and changing the yml config

@amannn
Copy link
Owner

amannn commented Nov 23, 2020

Interesting. Can you explain what exactly didn't work?

From how I understand the two events, I think these are the differences:

  • pull_request: This uses the latest workflow configuration, even if it is potentially changed in the branch. I currently use this in this repository to be able to see changes I make in a given branch, by referencing the local version of the action. The downside is, that the action doesn't work when a pull request is created from another repository (that's why the action failed in your PR here).
  • pull_request_target: This allows the action to be used in a fork-based workflow, where e.g. you want to accept PRs in a public repository. The action and configuration uses the latest configuration from master.

@bytestream
Copy link
Contributor Author

The workflow just didn't start at all. I'd push new commits, edit the title, etc. See proengsoft/laravel-jsvalidation#534

@amannn
Copy link
Owner

amannn commented Nov 23, 2020

Yep, as mentioned above, with pull_request_target you'll use the latest configuration from master. As the workflow wasn't in master at that point, there was no check to run.

I think if you have pull_request_target configured in master, the workflow will work for the next PR.

If you use pull_request (like I have currently configured in this repo), the check won't run for people who make a PR based on a fork (that's why the workflow failed in this PR).

That's at least my current understanding, I'm still wrapping my head around this topic 🙂. If this is correct, I think it should be documented in the README of this action.

@bytestream
Copy link
Contributor Author

Me too! OK thanks for clarifying. I'll try another PR with suggested change

@bytestream
Copy link
Contributor Author

bytestream commented Nov 23, 2020

with pull_request_target you'll use the latest configuration from master. As the workflow wasn't in master at that point, there was no check to run.

This seems like it was the issue. Thanks for the pointer.

Should I update this PR with a little note about it? If you have any suggestions on the wording I'd be grateful.

@amannn
Copy link
Owner

amannn commented Nov 23, 2020

@bytestream I've added a commit with a proposal for the improved README. Would you like to have a look and let me know if there's something to improve?

@amannn amannn changed the title chore: update example config docs: Clarify usage of event triggers in README Nov 23, 2020
@bytestream
Copy link
Contributor Author

Thanks. Looks good 👍

@amannn
Copy link
Owner

amannn commented Nov 23, 2020

Great, thanks for your help!

@amannn amannn merged commit 4dc9c78 into amannn:master Nov 23, 2020
@bytestream bytestream deleted the patch-1 branch November 23, 2020 17:32
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