You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@daffodil.apache.org by "Beckerle, Mike" <mb...@owlcyberdefense.com> on 2021/07/30 14:51:00 UTC
trying to rerun checks on PR - no option for it?
Darryl S. pushed a commit to his PR https://github.com/apache/daffodil/pull/601/checks
As a first time contributor, his checks won't automatically run.
I was going to trigger them manually, but I see no option for doing so.
Wasn't there a button for that? In the past I swear I saw one.
Anybody understand what's up with this before I open an INFRA ticket?
Mike Beckerle | Principal Engineer
[cid:9a6b4607-36da-4f6f-9218-104142fd8991]
mbeckerle@owlcyberdefense.com<ma...@owlcyberdefense.com>
P +1-781-330-0412
Re: trying to rerun checks on PR - no option for it?
Posted by Steve Lawrence <sl...@apache.org>.
On 8/24/21 3:55 PM, Beckerle, Mike wrote:
>
>>> Workflows will not run on pull_request activity if the pull request
>> has a merge conflict. The merge conflict must be resolved first.
>
>> I think users do just need to rebase and fix the conflicts.
>
> But that means the comment history of the PR will be lost, as one will have to force-push to the PR branch.
>
> I am thinking we could fix our workflow to address this. I am not sure it is worth it though.
>
> When creating a bug branch, also create a "point of departure" branch for the master branch point of departure.
> E.g, to fix bug 5555 create daf-5555-master from master, and then daf-5555-bug from daf-5555-master.
>
> PRs are created for merging back to this departure branch (daf-5555-master) and so will never have conflicts. So CI tests will always run.
>
> Feedback on the changes would then be independent of any conflicts with ongoing things merging to the master branch. Once all changes are complete, the changes can be squashed and merged back to the point-of-departure branch (daf-5555-master), and that branch can subsequently be rebased onto the master branch, which would involve fixing all conflicts.
>
> This is yet another step, and is only needed if a PR will be open and outstanding long enough to require it, but that's something one can't necessarily anticipate will be needed or not, so you'd have to just always work this way.
>
> I may try doing my next changes in this model.
One downside of this is that only committers can create new branches. So
non-committers would have to ask a committer to create a new branch that
they can merge the PR into, make sure they select the correct branch,
etc. All this just to open a PR seems like too much of a barrier to entry.
And then there's more work to merge a PR. We have to accept the PR into
the bug-master branch, then test that before actually merging into
master. We'd probably want that to be done via a PR as well to get the
CI checks. So two PR's are needed for a single change, and they aren't
really linked together in any way.
Also, looking the PR where this came up:
https://github.com/apache/daffodil/pull/601
all the previous comments do exist in the Conversation tab, some just
show as outdated. So things aren't completely lost. And my guess is
those "outdated" onces were on code that changed, so maybe the comments
aren't even relevant for the latest changes. Maybe GitHub fixed the
issue where force pushes lost comments, and this is a non-issue?
I'd also argue the problem of conflicts doesn't come up enough to
warrant extra complexity in the workflow. PR conflicts are pretty rare.
And keeping PR small avoids this, as well as makes for easier chunks to
review.
Re: trying to rerun checks on PR - no option for it?
Posted by "Beckerle, Mike" <mb...@owlcyberdefense.com>.
>> Workflows will not run on pull_request activity if the pull request
> has a merge conflict. The merge conflict must be resolved first.
> I think users do just need to rebase and fix the conflicts.
But that means the comment history of the PR will be lost, as one will have to force-push to the PR branch.
I am thinking we could fix our workflow to address this. I am not sure it is worth it though.
When creating a bug branch, also create a "point of departure" branch for the master branch point of departure.
E.g, to fix bug 5555 create daf-5555-master from master, and then daf-5555-bug from daf-5555-master.
PRs are created for merging back to this departure branch (daf-5555-master) and so will never have conflicts. So CI tests will always run.
Feedback on the changes would then be independent of any conflicts with ongoing things merging to the master branch. Once all changes are complete, the changes can be squashed and merged back to the point-of-departure branch (daf-5555-master), and that branch can subsequently be rebased onto the master branch, which would involve fixing all conflicts.
This is yet another step, and is only needed if a PR will be open and outstanding long enough to require it, but that's something one can't necessarily anticipate will be needed or not, so you'd have to just always work this way.
I may try doing my next changes in this model.
Re: trying to rerun checks on PR - no option for it?
Posted by Steve Lawrence <sl...@apache.org>.
Agreed, feels like checks should still run even if there are conflicts,
just with the understanding that the PR can't be merged until conflicts
are fixed. And then checks would be run against the merged commit once
the conflicts are fixed to ensure there are no failures once merged.
Unfortunately, this is how GitHub designed it, and from what I can tell
there's no way to control it. From:
https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request
> Workflows will not run on pull_request activity if the pull request
has a merge conflict. The merge conflict must be resolved first.
I think users do just need to rebase and fix the conflicts.
As to the dev branch issue, if we want to carry a PR on a separate dev
branch, then the PR should be changed to be merged into the dev branch
instead of the master branch. That way the PR checks will run as long as
there are no conflicts with the dev branch, which should hopefully be
less likely.
On 7/30/21 11:17 AM, Beckerle, Mike wrote:
> Ok, so to me this restriction is just plain wrong.
>
> A project that has multiple branches moving forward can't even work this way at all.
>
> The tests to run are well defined for this change set.
>
> What if we decided to never merge it back to master, but instead start carrying forward a dev branch separate from master?
>
> I don't want any contributor to have to rebase onto latest master until we see that their stuff works, passes the checks, etc. Rebasing on top of master will lose the history of comments on the PR. (all the commits change). That's also a "bug" not a feature, but to live with that we have to simply not rebase until very late in the game, and the current policy makes the automated CI testing only workable if you rebase onto master frequently, which we know is flawed.
>
> Am I incorrect here?
>
>
>
>
>
> ________________________________
> From: Interrante, John A (GE Research, US) <Jo...@ge.com>
> Sent: Friday, July 30, 2021 10:59 AM
> To: dev@daffodil.apache.org <de...@daffodil.apache.org>
> Subject: RE: trying to rerun checks on PR - no option for it?
>
> CI won't run when the PR has conflicts with the main branch. That's why the button isn't there. Darryl needs to rebase his PR, fix the conflict, and push his PR again with " git push --force-with-lease".
>
> From: Beckerle, Mike <mb...@owlcyberdefense.com>
> Sent: Friday, July 30, 2021 10:51 AM
> To: dev@daffodil.apache.org
> Subject: EXT: trying to rerun checks on PR - no option for it?
>
> Darryl S. pushed a commit to his PR https://github.com/apache/daffodil/pull/601/checks
>
> As a first time contributor, his checks won't automatically run.
>
> I was going to trigger them manually, but I see no option for doing so.
>
> Wasn't there a button for that? In the past I swear I saw one.
>
> Anybody understand what's up with this before I open an INFRA ticket?
>
>
> Mike Beckerle | Principal Engineer
>
> [cid:9a6b4607-36da-4f6f-9218-104142fd8991]
>
> mbeckerle@owlcyberdefense.com<ma...@owlcyberdefense.com>
> P +1-781-330-0412
>
>
Re: trying to rerun checks on PR - no option for it?
Posted by "Beckerle, Mike" <mb...@owlcyberdefense.com>.
Ok, so to me this restriction is just plain wrong.
A project that has multiple branches moving forward can't even work this way at all.
The tests to run are well defined for this change set.
What if we decided to never merge it back to master, but instead start carrying forward a dev branch separate from master?
I don't want any contributor to have to rebase onto latest master until we see that their stuff works, passes the checks, etc. Rebasing on top of master will lose the history of comments on the PR. (all the commits change). That's also a "bug" not a feature, but to live with that we have to simply not rebase until very late in the game, and the current policy makes the automated CI testing only workable if you rebase onto master frequently, which we know is flawed.
Am I incorrect here?
________________________________
From: Interrante, John A (GE Research, US) <Jo...@ge.com>
Sent: Friday, July 30, 2021 10:59 AM
To: dev@daffodil.apache.org <de...@daffodil.apache.org>
Subject: RE: trying to rerun checks on PR - no option for it?
CI won't run when the PR has conflicts with the main branch. That's why the button isn't there. Darryl needs to rebase his PR, fix the conflict, and push his PR again with " git push --force-with-lease".
From: Beckerle, Mike <mb...@owlcyberdefense.com>
Sent: Friday, July 30, 2021 10:51 AM
To: dev@daffodil.apache.org
Subject: EXT: trying to rerun checks on PR - no option for it?
Darryl S. pushed a commit to his PR https://github.com/apache/daffodil/pull/601/checks
As a first time contributor, his checks won't automatically run.
I was going to trigger them manually, but I see no option for doing so.
Wasn't there a button for that? In the past I swear I saw one.
Anybody understand what's up with this before I open an INFRA ticket?
Mike Beckerle | Principal Engineer
[cid:9a6b4607-36da-4f6f-9218-104142fd8991]
mbeckerle@owlcyberdefense.com<ma...@owlcyberdefense.com>
P +1-781-330-0412
RE: trying to rerun checks on PR - no option for it?
Posted by "Interrante, John A (GE Research, US)" <Jo...@ge.com>.
CI won't run when the PR has conflicts with the main branch. That's why the button isn't there. Darryl needs to rebase his PR, fix the conflict, and push his PR again with " git push --force-with-lease".
From: Beckerle, Mike <mb...@owlcyberdefense.com>
Sent: Friday, July 30, 2021 10:51 AM
To: dev@daffodil.apache.org
Subject: EXT: trying to rerun checks on PR - no option for it?
Darryl S. pushed a commit to his PR https://github.com/apache/daffodil/pull/601/checks
As a first time contributor, his checks won't automatically run.
I was going to trigger them manually, but I see no option for doing so.
Wasn't there a button for that? In the past I swear I saw one.
Anybody understand what's up with this before I open an INFRA ticket?
Mike Beckerle | Principal Engineer
[cid:9a6b4607-36da-4f6f-9218-104142fd8991]
mbeckerle@owlcyberdefense.com<ma...@owlcyberdefense.com>
P +1-781-330-0412