Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the include-all-prereleases input #127

Merged
merged 3 commits into from
Dec 12, 2022
Merged

Add the include-all-prereleases input #127

merged 3 commits into from
Dec 12, 2022

Conversation

SaschaMann
Copy link
Member

fixes #126

@SaschaMann SaschaMann requested a review from a team as a code owner November 24, 2022 21:17
Comment on lines +69 to +76
describe('include-prereleases', () => {
it('Chooses the highest available version that matches the input including prereleases', () => {
expect(installer.getJuliaVersion(testVersions, '^1.2.0-0', true)).toEqual('1.3.0-rc4')
expect(installer.getJuliaVersion(testVersions, '1', true)).toEqual('1.3.0-rc4')
expect(installer.getJuliaVersion(testVersions, '^1.2.0-0', false)).toEqual('1.2.0')
})
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend starting here for the review.

@SaschaMann
Copy link
Member Author

cc @EwoutH

@DilumAluthge
Copy link
Member

Can you explain why this is necessary? What's the use case here?

@DilumAluthge
Copy link
Member

DilumAluthge commented Nov 25, 2022

More specifically, when would you want to use this option but not want to just use either ^1.8.0-0 or ~1.8.0-0?

@SaschaMann
Copy link
Member Author

SaschaMann commented Nov 25, 2022

^1.8.0-0 would skip over 1.9.0-rc1 and jump to 1.9.0. With this input enabled, you could have a build that always uses the latest tagged release no matter what.

Are there many use cases for it? Not entirely convinced. It can save some time manually upgrading workflows I guess, it's a fairly straightforward change that doesn't introduce much complexity, and it's been requested by someone.

@DilumAluthge
Copy link
Member

I'm still not following why this is actually a useful thing to do.

@DilumAluthge
Copy link
Member

@EwoutH Could you elaborate on what your specific use case is here? What is the situation in which you want 1.8.0-rc1 or 1.9.0-rc1 but you don't want 1.9.0 if it's available?

@SaschaMann
Copy link
Member Author

What is the situation in which you want 1.8.0-rc1 or 1.9.0-rc1 but you don't want 1.9.0 if it's available?

I think I didn't describe it well: without this and a version 1.8.0-0, it would match the following versions once they are released: 1.8.0-rc1, 1.8.0, 1.9.0. With this flag, it would be 1.8.0-rc1, 1.8.0, 1.9.0-rc1, 1.9.0.

@DilumAluthge
Copy link
Member

Ohhhh now I get it. I actually thought that ^1.8.0-0 would already have included 1.8.0-rc1, 1.8.0, 1.9.0-rc1, and 1.9.0. But since it doesn't, I think this is a good feature to add.

@DilumAluthge
Copy link
Member

I don't know if include-prereleases is the best name for this feature? Because if the user passes ^1.8.0-0 and sets include-prereleases: false, it will still match 1.8.0-rc1 (which is a prerelease), right?

@DilumAluthge
Copy link
Member

I think I didn't describe it well: without this and a version 1.8.0-0, it would match the following versions once they are released: 1.8.0-rc1, 1.8.0, 1.9.0. With this flag, it would be 1.8.0-rc1, 1.8.0, 1.9.0-rc1, 1.9.0.

I think it would be good to include this exact example in the docs. This example is what made it "click" for me.

@SaschaMann
Copy link
Member Author

I don't know if include-prereleases is the best name for this feature? Because if the user passes ^1.8.0-0 and sets include-prereleases: false, it will still match 1.8.0-rc1 (which is a prerelease), right?

Yes, that's correct. Despite that it's also what node-semver uses. Do you have a suggestion?

I actually thought that ^1.8.0-0 would already have included 1.8.0-rc1, 1.8.0, 1.9.0-rc1, and 1.9.0.

Same.

@DilumAluthge
Copy link
Member

Hmmm. Maybe always-include-prereleases or always-allow-prereleases?

@EwoutH
Copy link

EwoutH commented Nov 28, 2022

@EwoutH Could you elaborate on what your specific use case is here?

My main use case is to have an early warning system in CI runs. So one job that always uses the latest Julia pre-release version, but is allowed to fail. So it can catch deprecations and warnings in Julia alpha, beta and rc versions.

@SaschaMann
Copy link
Member Author

SaschaMann commented Nov 28, 2022

Hmmm. Maybe always-include-prereleases or always-allow-prereleases?

Hm, those made me think the opposite of that is "sometimes" or "never" allowing prereleases, which isn't quite accurate. -0 also "always" includes pre-releases, just not all of them. But maybe include-all-prereleases?

@DilumAluthge
Copy link
Member

Yeah include-all-prereleases is definitely better than include-prereleases (in my opinion).

Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go after renaming the input to include-all-prereleases?

@EwoutH
Copy link

EwoutH commented Dec 11, 2022

I have one other suggestion, maybe we can create levels of pre-releases. For example:

  • include-prereleases: "all" (or "alpha") includes all pre-releases (alpha, beta, rc)
  • include-prereleases: "beta" includes beta and release-candidate pre-releases
  • include-prereleases: "rc" includes release-candidate pre-releases

@SaschaMann
Copy link
Member Author

That would require us to handle the version resolution ourselves rather than passing it to node's semver library which would introduce too much complexity to the action for (imo) little gain.

@SaschaMann SaschaMann enabled auto-merge (squash) December 11, 2022 14:48
@SaschaMann SaschaMann enabled auto-merge (squash) December 11, 2022 14:50
@DilumAluthge DilumAluthge changed the title Add include-prereleases input Add include-all-prereleases input Dec 11, 2022
@DilumAluthge DilumAluthge changed the title Add include-all-prereleases input Add the include-all-prereleases input Dec 12, 2022
@SaschaMann SaschaMann merged commit a7ad216 into master Dec 12, 2022
@SaschaMann SaschaMann deleted the force-prerelease branch December 12, 2022 03:03
@EwoutH
Copy link

EwoutH commented Dec 12, 2022

Awesome, thanks!

@SaschaMann
Copy link
Member Author

Tagged a release, should be available now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants