Get Plugin Releases - Prevent unstable releases

[puntope]

Changes proposed in this Pull Request:

Closes #101

This PR checks the release versions and ensures the maximum release version is the latest stable one (defined in "version" param under the JSON API .

Screenshots:

Local:

• The first command is before the PR
• Second after the PR

Detailed test instructions:

Verify https://github.com/woocommerce/google-listings-and-ads/actions/runs/8652279994/job/23724824443?pr=2365 is fetching the right versions

1. Clone this repo locally
2. Compile the code using npm run build (notice you might need to remove some GH output/input code)
3. Run node node get-plugin-releases.js
4. See it returns 8.8.0 as the latest
5. Checkout this PR
6. Repeat step 3
7. See it returns 8.7.0 as the latest

Additional details:

Changelog entry

Tweak - Prevent unstable releases to be returned

[jorgemd24]

Thanks @puntope for fixing this! I tested locally and it's returning the latest stable version. LGTM!

[mikkamp]

I tried to test this code with the instructions provided, but it wouldn't run with just node get-plugin-releases.js as it couldn't find the handle-actions and needed some of the input variables in particular slug=woocommerce to work. Do you have an easy way to get around that?

From testing I also noticed that I had to set both includeRC and includePatches to be able to include a RC version? Can those not be requested individually?

[mikkamp]

As far as I understand this will work if we aren't interested in RC versions. But if the stable version is at 8.7.0 and a 8.8.0-rc.1 is available it will now be filtered out.

[puntope]

Thanks @mikkamp for catching that. Added in a9ddff7

[puntope]

I tried to test this code with the instructions provided, but it wouldn't run with just node get-plugin-releases.js as it couldn't find the handle-actions and needed some of the input variables in particular slug=woocommerce to work. Do you have an easy way to get around that?

The way I do it is to hardcode the inputs directly. As the local env doesn't have the GH libraries.

[puntope]

From testing I also noticed that I had to set both includeRC and includePatches to be able to include a RC version? Can those not be requested individually?

I think that's not needed. You can set them individually.

includePatches will return all the patch versions founded for each minor.

Lets take ['8.8.0-rc.1', '8.8.0-rc.2', '8.7.0','8.6.1','8.6.2', '8.6.3' ] as the feched versions.

numberofReleases=6, includePatches=false, includeRC=true

result: [ '8.8.0-rc.2', '8.7.0', '8.6.3' ]

numberofReleases=6, includePatches=true, includeRC=false

result [ '8.7.0', '8.6.3', '8.6.2', '8.6.1' ]

numberofReleases=6, includePatches=true, includeRC=true

result [ '8.8.0-rc.2', '8.8.0-rc.1', '8.7.0', '8.6.3', '8.6.2', '8.6.1' ]

Can you elaborate a bit more about those test cases?

[puntope]

@mikkamp I updated the code to fix the RC issue. If its good feel free to merge it as I'm AFK next week.

I didn't see any issue with the includePatch/includeRC mentioned in #102 (comment). But if so, we can handle it ina. separate issue/pr

[mikkamp]

I didn't see any issue with the includePatch/includeRC mentioned in #102 (comment). But if so, we can handle it in a. separate issue/pr

Thanks for the clarification, however I don't seem to be having the same experience, which is because of the following if statement:

( includeRC || ! isRC( version ) ) &&
( includePatches || ! isMinorAlreadyAdded( output, version ) )

It requires both clauses to return a true. But the way the versions are ordered 8.7.0 comes before 8.7.0-rc.1 so if I have includeRC set to true and includePatches set to false, I'll get true && false because isMinorAlreadyAdded returns true. As 8.7 already occurs within output.
Anyways I agree we can handle that in a separate PR.

[mikkamp]

Thanks for the additional changes. This now works when we have includeRC set to false it only returns 8.7.0 as the latest version. However if we set includeRC to true it returns both 8.8.0 and 8.8.0-rc.1
I'd be expecting it to return only the 8.8.0-rc.1, should it be using isRC instead of includeRC?

[puntope]

You're right @mikkamp it should be checked with isRC function. I tweaked it here 8db7d34

[puntope]

I didn't see any issue with the includePatch/includeRC mentioned in #102 (comment). But if so, we can handle it in a. separate issue/pr

Thanks for the clarification, however I don't seem to be having the same experience, which is because of the following if statement:

( includeRC || ! isRC( version ) ) &&
( includePatches || ! isMinorAlreadyAdded( output, version ) )

It requires both clauses to return a true. But the way the versions are ordered 8.7.0 comes before 8.7.0-rc.1 so if I have includeRC set to true and includePatches set to false, I'll get true && false because isMinorAlreadyAdded returns true. As 8.7 already occurs within output. Anyways I agree we can handle that in a separate PR.

Hi @mikkamp as for this, now I can reproduce the "issue" thanks. However, it's really an issue? The idea of that param is to get RC version for testing the versions before being released. Let's take 8.9.0-rc.1 as the last RC version available. And 8.8.0 as the latest stable. Then includeRC=true will work without setting includePatches=true as expected.

When 8.9.0 stable is there. We don't really need to test the RC for that version, as the latest version would be 8.9.0 which is newer that the RC.

[puntope]

Hi @mikkamp I fixed the last mentioned bug. Can you take another look? 🙏 This is how I tested it.

test 1

{
  version: '8.8.0',
  versions: {
    '8.9.0': '',
    '8.8.0': '',
    '8.7.4': '',
    '8.7.3': '',
    '8.7.0': '',
    '8.6.5': '',
    '8.6.4': ''
  }
}
includeRC true
includePatches false
==> Output "versions":
8.8.0,8.7.4,8.6.5

test  2
{
  version: '8.8.0',
  versions: {
    '8.9.0': '',
    '8.8.0': '',
    '8.7.4': '',
    '8.7.3': '',
    '8.7.0': '',
    '8.6.5': '',
    '8.6.4': ''
  }
}
includeRC true
includePatches true
==> Output "versions":
8.8.0,8.7.4,8.7.3,8.7.0,8.6.5,8.6.4

test  3
{
  version: '8.8.0',
  versions: {
    '8.9.0': '',
    '8.8.0': '',
    '8.7.4': '',
    '8.7.3': '',
    '8.7.0': '',
    '8.6.5': '',
    '8.6.4': ''
  }
}
includeRC false
includePatches true
==> Output "versions":
8.8.0,8.7.4,8.7.3,8.7.0,8.6.5,8.6.4

test 4
{
  version: '8.8.0',
  versions: {
    '8.9.0-rc.2': '',
    '8.9.0-rc.1': '',
    '8.8.0': '',
    '8.7.4': '',
    '8.7.3': '',
    '8.7.0': '',
    '8.6.5': '',
    '8.6.4': ''
  }
}
includeRC true
includePatches false
==> Output "versions":
8.9.0-rc.2,8.8.0,8.7.4,8.6.5

In regards the issue with the includeRC/includePatches. See my comments here

[mikkamp]

Thanks for the additional fixes. It's now returning the "latest" release candidate when it's available.

In regards to it returning only the "latest" release candidate when includePatches is false, that's fine, but it wasn't clear to me what those were supposed to return. Maybe describing them in the readme would be helpful.

The example "Get L-3 Release versions from WC including RC" is also not super clear how this works. During the times when there is a "latest" release candidate available it will return latest RC, current stable, L-1, L-2. When there is no release candidate prepared yet it will return current stable, L-1, L-2, L-3. Maybe clarifying that in the docs will help clear up any confusion in the future.