You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Robert Houghton <rh...@vmware.com> on 2021/12/16 18:08:07 UTC
[DISCUSS] Adding LTGM as gating PR checks
We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
Does anyone have strong feelings against that?
-Robert Houghton
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Kirk Lund <kl...@apache.org>.
I support making LGTM a gating job on every PR.
On Thu, Dec 16, 2021 at 2:46 PM Alexander Murmann <am...@vmware.com>
wrote:
> +1 to adding this. Given it has a low false-positive rate, only checks on
> code actually changed in the PR and runs quickly compared to some of our
> other steps that already happen for every PR, this seems like an easy
> decision.
>
> ________________________________
> From: Robert Houghton <rh...@vmware.com>
> Sent: Thursday, December 16, 2021 14:20
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [DISCUSS] Adding LTGM as gating PR checks
>
> Short answer would be to work with the rest of the community to get the
> check to pass, fix the LGTM configuration, something like that. Otherwise,
> the Concourse CI has the ability to set PR context messages.
>
> -Robert
>
> From: Owen Nichols <on...@vmware.com>
> Date: Thursday, December 16, 2021 at 10:40 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [DISCUSS] Adding LTGM as gating PR checks
> Requiring LGTM looks good to me. It does not seem to have a high rate of
> false-positives like some other linters, but if we are making it gating,
> what would the process look like to override a false-positive?
>
> On 12/16/21, 10:37 AM, "Anthony Baker" <ba...@vmware.com> wrote:
>
> Thanks Robert, I think this is important. I think this is a good first
> step.
>
> In future I think we should consider adding a CI job to ensure that
> pre-existing security errors are addressed. Perhaps GitHub code scanning is
> worth investigating since they have acquired the LGTM product.
>
> Anthony
>
>
> > On Dec 16, 2021, at 10:08 AM, Robert Houghton <rh...@vmware.com>
> wrote:
> >
> > We have had LGTM tests enabled on Apache Geode PRs for quite some
> time, and have done a great job of trending those warnings and errors to in
> the right direction. I would like to make the change to our GitHub to make
> those changes blocking for all new PRs, given their reliability and
> lack-of-flakiness.
> >
> > Does anyone have strong feelings against that?
> >
> > -Robert Houghton
>
>
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Alexander Murmann <am...@vmware.com>.
+1 to adding this. Given it has a low false-positive rate, only checks on code actually changed in the PR and runs quickly compared to some of our other steps that already happen for every PR, this seems like an easy decision.
________________________________
From: Robert Houghton <rh...@vmware.com>
Sent: Thursday, December 16, 2021 14:20
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [DISCUSS] Adding LTGM as gating PR checks
Short answer would be to work with the rest of the community to get the check to pass, fix the LGTM configuration, something like that. Otherwise, the Concourse CI has the ability to set PR context messages.
-Robert
From: Owen Nichols <on...@vmware.com>
Date: Thursday, December 16, 2021 at 10:40 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [DISCUSS] Adding LTGM as gating PR checks
Requiring LGTM looks good to me. It does not seem to have a high rate of false-positives like some other linters, but if we are making it gating, what would the process look like to override a false-positive?
On 12/16/21, 10:37 AM, "Anthony Baker" <ba...@vmware.com> wrote:
Thanks Robert, I think this is important. I think this is a good first step.
In future I think we should consider adding a CI job to ensure that pre-existing security errors are addressed. Perhaps GitHub code scanning is worth investigating since they have acquired the LGTM product.
Anthony
> On Dec 16, 2021, at 10:08 AM, Robert Houghton <rh...@vmware.com> wrote:
>
> We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
>
> Does anyone have strong feelings against that?
>
> -Robert Houghton
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Robert Houghton <rh...@vmware.com>.
Short answer would be to work with the rest of the community to get the check to pass, fix the LGTM configuration, something like that. Otherwise, the Concourse CI has the ability to set PR context messages.
-Robert
From: Owen Nichols <on...@vmware.com>
Date: Thursday, December 16, 2021 at 10:40 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [DISCUSS] Adding LTGM as gating PR checks
Requiring LGTM looks good to me. It does not seem to have a high rate of false-positives like some other linters, but if we are making it gating, what would the process look like to override a false-positive?
On 12/16/21, 10:37 AM, "Anthony Baker" <ba...@vmware.com> wrote:
Thanks Robert, I think this is important. I think this is a good first step.
In future I think we should consider adding a CI job to ensure that pre-existing security errors are addressed. Perhaps GitHub code scanning is worth investigating since they have acquired the LGTM product.
Anthony
> On Dec 16, 2021, at 10:08 AM, Robert Houghton <rh...@vmware.com> wrote:
>
> We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
>
> Does anyone have strong feelings against that?
>
> -Robert Houghton
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Owen Nichols <on...@vmware.com>.
Requiring LGTM looks good to me. It does not seem to have a high rate of false-positives like some other linters, but if we are making it gating, what would the process look like to override a false-positive?
On 12/16/21, 10:37 AM, "Anthony Baker" <ba...@vmware.com> wrote:
Thanks Robert, I think this is important. I think this is a good first step.
In future I think we should consider adding a CI job to ensure that pre-existing security errors are addressed. Perhaps GitHub code scanning is worth investigating since they have acquired the LGTM product.
Anthony
> On Dec 16, 2021, at 10:08 AM, Robert Houghton <rh...@vmware.com> wrote:
>
> We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
>
> Does anyone have strong feelings against that?
>
> -Robert Houghton
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Anthony Baker <ba...@vmware.com>.
Thanks Robert, I think this is important. I think this is a good first step.
In future I think we should consider adding a CI job to ensure that pre-existing security errors are addressed. Perhaps GitHub code scanning is worth investigating since they have acquired the LGTM product.
Anthony
> On Dec 16, 2021, at 10:08 AM, Robert Houghton <rh...@vmware.com> wrote:
>
> We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
>
> Does anyone have strong feelings against that?
>
> -Robert Houghton
Re: [DISCUSS] Adding LTGM as gating PR checks
Posted by Robert Houghton <rh...@vmware.com>.
Excuse me. I meant to link the PR that would enable this behavior: https://github.com/apache/geode/pull/7197
-Robert Houghton
From: Robert Houghton <rh...@vmware.com>
Date: Thursday, December 16, 2021 at 10:08 AM
To: geode <de...@geode.apache.org>
Subject: [DISCUSS] Adding LTGM as gating PR checks
We have had LGTM tests enabled on Apache Geode PRs for quite some time, and have done a great job of trending those warnings and errors to in the right direction. I would like to make the change to our GitHub to make those changes blocking for all new PRs, given their reliability and lack-of-flakiness.
Does anyone have strong feelings against that?
-Robert Houghton