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

Run PR activities on push on non-default branch #248

Merged
merged 9 commits into from
Sep 13, 2023
Merged

Conversation

ewjoachim
Copy link
Member

@ewjoachim ewjoachim commented Sep 3, 2023

Closes #234, Closes #246

Still needs:

  • Manual tests
  • A decision regarding whether we need e2e tests for that or not
  • Documentation (especially on how to write the part that posts the comment from the artifact. Though it will be very much like CommentPR.yml from here)

But the code is written, and we already have 100% coverage so that's a good start.

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

End-to-end public repo

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Coverage report

The coverage rate went from 100% to 100% ➡️
The branch rate is 100%.

100% of new lines are covered.

Diff Coverage details (click to unfold)

coverage_comment/activity.py

100% of new lines are covered (100% of the complete file).

coverage_comment/main.py

100% of new lines are covered (100% of the complete file).

coverage_comment/settings.py

100% of new lines are covered (100% of the complete file).

coverage_comment/github.py

100% of new lines are covered (100% of the complete file).

@kieferro kieferro self-requested a review September 4, 2023 01:31
Copy link
Member

@kieferro kieferro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, looks good 👍. I still have a few small comments, but I tested with a copy of the template repo and everything seems to work fine.

coverage_comment/main.py Outdated Show resolved Hide resolved
coverage_comment/main.py Outdated Show resolved Hide resolved
coverage_comment/main.py Show resolved Hide resolved
coverage_comment/main.py Show resolved Hide resolved
@ewjoachim
Copy link
Member Author

Comments addressed. For the question of the default branch, I've:

  • removed previous coverage data when running on a PR not targeting the default branch
  • but kept it in case we're running on a branch with no PR yet: we can't know.

@ewjoachim
Copy link
Member Author

I've also changed the Note that displays where there's no reference coverage info to use a specific message in the case it's because the PR doesn't target the default branch

@kieferro kieferro self-requested a review September 13, 2023 00:48
Copy link
Member

@kieferro kieferro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

A decision regarding whether we need e2e tests for that or not

I'm not quite sure if this is so easy to test if we also want to test the normal use cases and therefore want to keep pull_request as a trigger. We could build some system that this trigger is ignored for example if the PR title contains a certain word and we create a PR which contains that word and tests if it also works only through the push trigger, but I don't know if that's worth the effort. What's your assessment?

@ewjoachim
Copy link
Member Author

What's your assessment?

I agree with you :)

@ewjoachim ewjoachim merged commit 211af00 into main Sep 13, 2023
3 checks passed
@ewjoachim ewjoachim deleted the action-on-push branch September 13, 2023 15:09
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.

Switch to new note syntax Check for open PRs on push GHA trigger
2 participants