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

ci: run ci for all PRs, not only those targeting main #281

Closed
wants to merge 1 commit into from

Conversation

roobre
Copy link
Collaborator

@roobre roobre commented Aug 5, 2023

Description

Sometimes it is useful to stack PRs to make reviewing them easier, specially as Github is kind enough to automatically repoint them to main when the base PR gets merged.

Currently, PRs not pointing to main do not get tested. This PR proposes changing that to allow for this kind of workflow.

@pablochacin
Copy link
Collaborator

Sometimes it is useful to stack PRs to make reviewing them easier

I don't oppose to enable this feature, but I'm not sure I follow the reasoning of the justification for this change: "reviewing them easier". I'm generally not in favor of stacking PR for review, but only for developers to start follow-up work (e.g. draft a PR) while one PR is still in review. Could you please elaborate on this point?

@roobre
Copy link
Collaborator Author

roobre commented Aug 23, 2023

reviewing them easier

By this I mean that the diff is easier to look at, specially for self-review.

I agree that I don't see any use case for merging one PR into another, but I think it does help for visualizing the changes.

Copy link
Collaborator

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

I'm still unconvinced opening PR against other branches is a good practice in the context of this project. However, I don't see any obvious risks or drawbacks.

@roobre
Copy link
Collaborator Author

roobre commented Aug 25, 2023

So far I have only found this need twice, so perhaps it's not worth changing. I can close this for now and bring it up again if it becomes an issue again.

@roobre roobre closed this Aug 25, 2023
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