-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)coverage_comment/activity.py
coverage_comment/main.py
coverage_comment/settings.py
coverage_comment/github.py
|
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.
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.
487ac04
to
cdf71fa
Compare
Comments addressed. For the question of the default branch, I've:
|
cdf71fa
to
ea13912
Compare
cb694a7
to
66ad832
Compare
I've also changed 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.
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?
I agree with you :) |
Closes #234, Closes #246
Still needs:
But the code is written, and we already have 100% coverage so that's a good start.