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

Fix how the author association is looked up for PRs. #135

Merged
merged 2 commits into from
May 18, 2024

Conversation

plietar
Copy link
Contributor

@plietar plietar commented May 17, 2024

The use_touchstone_workflows was generating code that used the github.event.comment.author_association regardless of the trigger type. However this does not work for pull_request triggers, meaning the if condition would always fail to evaluate and the workflow never ran. This commit fixes that by using the correct field name.

The use_touchstone_workflows was generating code that used the
`github.event.comment.author_association` regardless of the trigger
type. However this does not work for pull_request triggers, meaning the
if condition would always fail to evaluate and the workflow never ran.
This commit fixes that by using the correct field name.
@plietar
Copy link
Contributor Author

plietar commented May 17, 2024

See https://github.com/plietar/touchstone-test/actions/runs/9128401168/workflow for a workflow run that got skipped, even though it was opened by the repo owner.

@lorenzwalthert
Copy link
Owner

Thanks. Did you test this PR already as the other one with your workflow? I think we also lack proper integration tests in this repo. Would be a great addition so we can merge non-trivial, hard to test changes like this one with more confidence.

@plietar
Copy link
Contributor Author

plietar commented May 17, 2024

Yes this workflow run uses the change: https://github.com/plietar/touchstone-test/actions/runs/9128351202
It got triggered by opening the PR.

(The workflow got stuck when installing dependencies for some reason I'm still trying to debug, but that should be unrelated I updated the OS version and RSPM and that solved my workflows getting stuck)

I agree integration tests would be nice, but I don't really know if we can run actions as part of the CI. In the meantime the snapshots are at least a good way to see what gets produced as a workflow file.

@assignUser
Copy link
Collaborator

Ah good catch, thanks.

if we can run actions as part of the CI

Yes you can run repo-local actions via - uses: ./path/to/action here is an example. Proper tests would be great!

@assignUser assignUser merged commit 42c3630 into lorenzwalthert:main May 18, 2024
6 checks passed
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

3 participants