You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by Michael Bien <mb...@gmail.com> on 2024/11/04 19:28:22 UTC
[DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Hello NetBeans devs,
PR reviews and merges:
In the last weeks we had: a new "noreply" mail merged (first time
contributor), at least two merges to delivery from outside of the
release team and a PR merged where CI didn't run. This was noticed when
master failed which had to be fixed afterwards.
This all could have been avoided with a quick look at the pre-merge
check list before pressing the button. (the author/email validation can
be hopefully automated, see below)
I would also like to update the reviewer guide with another note:
reviewers should not change the PR target without also leaving a comment
on the PR. We had at least one situation where the committer who merged
did likely not notice that the target changed! Further: retarget will
almost always also require rebaseing of the branch + new CI run after
rebase + new manual testing (review!) in case of delivery integration -
so this _can't_ be simply changed without communication.
here the links to the relevant wiki pages as reminder:
https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
https://cwiki.apache.org/confluence/display/NETBEANS/Pull+requests+for+delivery
(guide does also link to this page)
commit header validation during CI:
I re-tested and updated yesterday an old PR which is an attempt to
implement basic commit header validation. We talked about this on
several occasions (usually after something happened, directly in the PR
comments or ASF slack), it will fail the "paperwork" job on
github-typical "noreply" mails, single-word author names (indicator for
github account name) and empty or single word commit messages (indicator
for unsquashed PR accidents about to happen).
This is only there to prevent the most common accidents/mistakes, it
will not actually validate emails or anything like that.
This is imperfect of course and will cause the paperwork job to fail
more often (which is fine), esp when the PR is still in development.
This is why the paperwork job will now no longer prevent CI workspace
cleanup from taking place on failure.
https://github.com/apache/netbeans/pull/6630
planning to merge the PR soon - if possible - but there is still time
for feedback,
please take another look at the reviewer guide and delivery merge rules
for those who haven't in a while,
best regards,
michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Posted by Matthias Bläsing <mb...@doppel-helix.eu.INVALID>.
Hi,
Am Montag, dem 04.11.2024 um 20:28 +0100 schrieb Michael Bien:
> I would also like to update the reviewer guide with another note:
> reviewers should not change the PR target without also leaving a comment
> on the PR. We had at least one situation where the committer who merged
> did likely not notice that the target changed! Further: retarget will
> almost always also require rebaseing of the branch + new CI run after
> rebase + new manual testing (review!) in case of delivery integration -
> so this _can't_ be simply changed without communication.
this makes sense to me.
> commit header validation during CI:
>
> I re-tested and updated yesterday an old PR which is an attempt to
> implement basic commit header validation. We talked about this on
> several occasions (usually after something happened, directly in the PR
> comments or ASF slack), it will fail the "paperwork" job on
> github-typical "noreply" mails, single-word author names (indicator for
> github account name) and empty or single word commit messages (indicator
> for unsquashed PR accidents about to happen).
>
> This is only there to prevent the most common accidents/mistakes, it
> will not actually validate emails or anything like that.
>
> This is imperfect of course and will cause the paperwork job to fail
> more often (which is fine), esp when the PR is still in development.
> This is why the paperwork job will now no longer prevent CI workspace
> cleanup from taking place on failure.
>
> https://github.com/apache/netbeans/pull/6630
I agree with the idea and validated, that it works in the confines,
that it can. I already expressed, that I don't like the usage of
experimental features, but that is a side discussion, that does not
invalidate the usefullness of the change.
From my POV this is good to go.
Thank you
Matthias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Posted by Jaroslav Tulach <ja...@gmail.com>.
Dne sobota 9. listopadu 2024 10:41:16 CET, Neil C Smith napsal(a):
> Change of branch to delivery *requires* reviews to be re-done - let's
> make that more explicit than "typically". Anyone changing the target
> branch should also make sure that any previous approvals are
> re-requested at the same time.
Sensible.
-jt
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Posted by Michael Bien <mb...@gmail.com>.
On 09.11.24 10:41, Neil C Smith wrote:
> On Sat, 9 Nov 2024 at 08:46, Michael Bien <mb...@gmail.com> wrote:
>> multiple choice:
>> a) should we give up on commit author validation and remove this line?
>> https://github.com/apache/netbeans/blob/7d89336259b840ce53709cbc8240c88e32fd6ea1/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L13
>> b) try validation during CI as proposed in the PR
>> c) do something else?
> Option b) I think unless we decide, against previous decisions, that
> we don't care about accurate author information in the git history.
> Either we care about this or we don't!
>
>> reviewer guide:
>> may I add a note to the reviewer guide in the sense of:
>> "Committers shall avoid changing the PR target branch without leaving a
>> comment. Changing the target typically will also require a rebase, CI
>> run and renewed reviewer approval."
> Please do! We need to reduce the chance of situations like the recent
> accidental merges to delivery happening again in future.
>
> Change of branch to delivery *requires* reviews to be re-done - let's
> make that more explicit than "typically". Anyone changing the target
> branch should also make sure that any previous approvals are
> re-requested at the same time.
I added this section to the "Pull requests for delivery" page:
- - -
Notes on retargeting existing PRs from master to delivery (or vice versa):
* Committers should not change the PR target branch without leaving a
comment. Changing the target will require to rebase the branch, a fresh
CI run and renewed reviewer approval.
* Please renew the review requests if the PR target is modified.
- - -
Feel free to change the wording if you like.
The commit header validator is now also merged and should be active for
fresh CI runs. If anyone is seeing any false positives or if it is
disrupting your workflow - please let me know.
> We could possibly even enforce or
> automate some of this via GH Actions quite simply, assuming there's a
> trigger for base branch changes?
there might be a trigger for PR target changes. But our CI can't do a
lot with this information. Anything interesting would require write
permissions to the PR. (leave a comment to ping reviewers or something
like that).
I wouldn't want to mess with permissions unless it can't be avoided. We
probably also can't change them without talking to infra.
>
> Likewise, we could add here that following [NOTICE] emails on dev@ is
> a *requirement* for active committers. This is particularly important
> for anyone merging to any branch on the repository. We should perhaps
> remind all committers of this?
we could write a [NOTICE] mail ;)
best regards,
michael
>
> Best wishes,
>
> Neil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Posted by Neil C Smith <ne...@apache.org>.
On Sat, 9 Nov 2024 at 08:46, Michael Bien <mb...@gmail.com> wrote:
> multiple choice:
> a) should we give up on commit author validation and remove this line?
> https://github.com/apache/netbeans/blob/7d89336259b840ce53709cbc8240c88e32fd6ea1/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L13
> b) try validation during CI as proposed in the PR
> c) do something else?
Option b) I think unless we decide, against previous decisions, that
we don't care about accurate author information in the git history.
Either we care about this or we don't!
> reviewer guide:
> may I add a note to the reviewer guide in the sense of:
> "Committers shall avoid changing the PR target branch without leaving a
> comment. Changing the target typically will also require a rebase, CI
> run and renewed reviewer approval."
Please do! We need to reduce the chance of situations like the recent
accidental merges to delivery happening again in future.
Change of branch to delivery *requires* reviews to be re-done - let's
make that more explicit than "typically". Anyone changing the target
branch should also make sure that any previous approvals are
re-requested at the same time. We could possibly even enforce or
automate some of this via GH Actions quite simply, assuming there's a
trigger for base branch changes?
Likewise, we could add here that following [NOTICE] emails on dev@ is
a *requirement* for active committers. This is particularly important
for anyone merging to any branch on the repository. We should perhaps
remind all committers of this?
Best wishes,
Neil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [DISCUSS] CI: commit header validation for PRs and PR reviewer guide reminder
Posted by Michael Bien <mb...@gmail.com>.
ping
multiple choice:
a) should we give up on commit author validation and remove this line?
https://github.com/apache/netbeans/blob/7d89336259b840ce53709cbc8240c88e32fd6ea1/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L13
b) try validation during CI as proposed in the PR
c) do something else?
reviewer guide:
may I add a note to the reviewer guide in the sense of:
"Committers shall avoid changing the PR target branch without leaving a
comment. Changing the target typically will also require a rebase, CI
run and renewed reviewer approval."
-mbien
On 04.11.24 20:28, Michael Bien wrote:
> Hello NetBeans devs,
>
>
> PR reviews and merges:
>
> In the last weeks we had: a new "noreply" mail merged (first time
> contributor), at least two merges to delivery from outside of the
> release team and a PR merged where CI didn't run. This was noticed
> when master failed which had to be fixed afterwards.
>
> This all could have been avoided with a quick look at the pre-merge
> check list before pressing the button. (the author/email validation
> can be hopefully automated, see below)
>
> I would also like to update the reviewer guide with another note:
> reviewers should not change the PR target without also leaving a
> comment on the PR. We had at least one situation where the committer
> who merged did likely not notice that the target changed! Further:
> retarget will almost always also require rebaseing of the branch + new
> CI run after rebase + new manual testing (review!) in case of delivery
> integration - so this _can't_ be simply changed without communication.
>
> here the links to the relevant wiki pages as reminder:
>
> https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
>
>
> https://cwiki.apache.org/confluence/display/NETBEANS/Pull+requests+for+delivery
> (guide does also link to this page)
>
>
> commit header validation during CI:
>
> I re-tested and updated yesterday an old PR which is an attempt to
> implement basic commit header validation. We talked about this on
> several occasions (usually after something happened, directly in the
> PR comments or ASF slack), it will fail the "paperwork" job on
> github-typical "noreply" mails, single-word author names (indicator
> for github account name) and empty or single word commit messages
> (indicator for unsquashed PR accidents about to happen).
>
> This is only there to prevent the most common accidents/mistakes, it
> will not actually validate emails or anything like that.
>
> This is imperfect of course and will cause the paperwork job to fail
> more often (which is fine), esp when the PR is still in development.
> This is why the paperwork job will now no longer prevent CI workspace
> cleanup from taking place on failure.
>
> https://github.com/apache/netbeans/pull/6630
>
>
> planning to merge the PR soon - if possible - but there is still time
> for feedback,
>
> please take another look at the reviewer guide and delivery merge
> rules for those who haven't in a while,
>
> best regards,
>
> michael
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists