Get Plugin Releases - Prevent unstable releases

puntope commented 4 months ago

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

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

Additional details:

Changelog entry

Tweak - Prevent unstable releases to be returned

puntope commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 https://github.com/woocommerce/grow/pull/102#issuecomment-2053269819. But if so, we can handle it ina. separate issue/pr

mikkamp commented 4 months ago

I didn't see any issue with the includePatch/includeRC mentioned in https://github.com/woocommerce/grow/pull/102#issuecomment-2053269819. 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.

puntope commented 4 months ago

I didn't see any issue with the includePatch/includeRC mentioned in https://github.com/woocommerce/grow/pull/102#issuecomment-2053269819. 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 commented 4 months ago

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

woocommerce / grow