Re: [DISCUSS] proposal to pare down old-version testing
Yes! This! 100%! > On Dec 22, 2021, at 7:14 PM, Owen Nichols wrote: > > Since adopting our N-2 support policy, the list of released versions in > /settings.gradle has ballooned to over 30 entries [1]. > > CI tests use this list to confirm that we don’t break rolling upgrade ability > or compatibility with older clients, but some of these tests don’t seem to > scale well: PR#7203 to add the most recent 3 releases (bringing the total to > 33) is unable to pass CI after 8 tries. > > Possible solutions fall into two categories: keep the full list and throw > developers and/or more hardware at the struggling tests, or concede that > testing every version is not a scalable approach and find ways to shorten the > list, e.g. randomly select a subset of old versions at runtime, or manually > pare down the list. > > I propose to shorten the list [2] by keeping only the latest patch for each > minor (unless the client or server protocol version has changed, so also keep > the patch prior to 1.12.1 and prior to 1.13.2). As long as a patch release > doesn’t change the client or server protocol version, I see low value in > testing upgrades from every patch version to every future version forever. > The months between patch releases already provide plenty of upgrade coverage > on that specific patch, then we can move on to the next…even if there could > somehow be a corner-case where transitive property of upgradability doesn’t > hold, most users probably take the latest-to-latest upgrade path anyway, > which will always be tested. > > Let’s keep discussion open until 3PM PST Jan 5. In case of no response, I > will assume lazy consensus and update settings.gradle as proposed [2]. > > > > [1] Current list from > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fsettings.gradle%23L72-L101data=04%7C01%7Cjabarrett%40vmware.com%7C1699e45bec75493de2b008d9c5c243c5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637758260417697506%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=qdmkkxmOrSZrjWEzS%2Fmjx6kEqBENrBxKBfBHE7ZiA30%3Dreserved=0 > : > 1.0.0-incubating > 1.1.0 > 1.1.1 > 1.2.0 > 1.3.0 > 1.4.0 > 1.5.0 > 1.6.0 > 1.7.0 > 1.8.0 > 1.9.0 > 1.9.1 > 1.9.2 > 1.10.0 > 1.11.0 > 1.12.0 > 1.12.1 > 1.12.2 > 1.12.3 > 1.12.4 > 1.12.5 > 1.12.6 > 1.12.7* > 1.13.0 > 1.13.1 > 1.13.2 > 1.13.3 > 1.13.4 > 1.13.5 > 1.13.6* > 1.14.0 > 1.14.1 > 1.14.2* > *=released, but not yet added to settings.gradle due to PR#7203 not able to > pass CI due to size of version list > > [2] Proposed shortlist: > 1.1.1 > 1.2.0 > 1.3.0 > 1.4.0 > 1.5.0 > 1.6.0 > 1.7.0 > 1.8.0 > 1.9.2 > 1.10.0 > 1.11.0 > 1.12.0 > 1.12.7 > 1.13.1 > 1.13.6 > 1.14.2 >
[DISCUSS] proposal to pare down old-version testing
Since adopting our N-2 support policy, the list of released versions in /settings.gradle has ballooned to over 30 entries [1]. CI tests use this list to confirm that we don’t break rolling upgrade ability or compatibility with older clients, but some of these tests don’t seem to scale well: PR#7203 to add the most recent 3 releases (bringing the total to 33) is unable to pass CI after 8 tries. Possible solutions fall into two categories: keep the full list and throw developers and/or more hardware at the struggling tests, or concede that testing every version is not a scalable approach and find ways to shorten the list, e.g. randomly select a subset of old versions at runtime, or manually pare down the list. I propose to shorten the list [2] by keeping only the latest patch for each minor (unless the client or server protocol version has changed, so also keep the patch prior to 1.12.1 and prior to 1.13.2). As long as a patch release doesn’t change the client or server protocol version, I see low value in testing upgrades from every patch version to every future version forever. The months between patch releases already provide plenty of upgrade coverage on that specific patch, then we can move on to the next…even if there could somehow be a corner-case where transitive property of upgradability doesn’t hold, most users probably take the latest-to-latest upgrade path anyway, which will always be tested. Let’s keep discussion open until 3PM PST Jan 5. In case of no response, I will assume lazy consensus and update settings.gradle as proposed [2]. [1] Current list from https://github.com/apache/geode/blob/develop/settings.gradle#L72-L101 : 1.0.0-incubating 1.1.0 1.1.1 1.2.0 1.3.0 1.4.0 1.5.0 1.6.0 1.7.0 1.8.0 1.9.0 1.9.1 1.9.2 1.10.0 1.11.0 1.12.0 1.12.1 1.12.2 1.12.3 1.12.4 1.12.5 1.12.6 1.12.7* 1.13.0 1.13.1 1.13.2 1.13.3 1.13.4 1.13.5 1.13.6* 1.14.0 1.14.1 1.14.2* *=released, but not yet added to settings.gradle due to PR#7203 not able to pass CI due to size of version list [2] Proposed shortlist: 1.1.1 1.2.0 1.3.0 1.4.0 1.5.0 1.6.0 1.7.0 1.8.0 1.9.2 1.10.0 1.11.0 1.12.0 1.12.7 1.13.1 1.13.6 1.14.2
Re: [DISCUSS] Adding LTGM as gating PR checks
I support making LGTM a gating job on every PR. On Thu, Dec 16, 2021 at 2:46 PM Alexander Murmann 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 > Sent: Thursday, December 16, 2021 14:20 > To: dev@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 > Date: Thursday, December 16, 2021 at 10:40 AM > To: dev@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" 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 > 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 > >
