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

Include include-prerelease option to be able to match latest prerelease versions #110

Merged
merged 4 commits into from
May 5, 2021
Merged

Include include-prerelease option to be able to match latest prerelease versions #110

merged 4 commits into from
May 5, 2021

Conversation

GGG-KILLER
Copy link
Contributor

@GGG-KILLER GGG-KILLER commented Jul 16, 2020

This is an implementation of the includePrerelease flag as discussed in #105.
Fixes #105.

@GGG-KILLER
Copy link
Contributor Author

So @ZEisinger, I'm not sure how would be the best way to test this since the .NET versions fetching isn't deterministic in tests and 5.0 isn't going to be in prerelease for much longer...

@GGG-KILLER GGG-KILLER marked this pull request as ready for review July 17, 2020 20:01
@GGG-KILLER GGG-KILLER changed the title WIP: Include include-prerelease option to be able to match latest prerelease versions Include include-prerelease option to be able to match latest prerelease versions Jul 21, 2020
@ZEisinger ZEisinger mentioned this pull request Sep 3, 2020
@WeihanLi
Copy link

When'll this pull request merged? Really hope for this great feature

@GGG-KILLER
Copy link
Contributor Author

GGG-KILLER commented Oct 20, 2020

When'll this pull request merged? Really hope for this great feature

I don't know... At this point I'm just waiting on @ZEisinger

@afucher
Copy link

afucher commented Feb 17, 2021

This will probably fix #170
any news @ZEisinger / @GGG-KILLER ?

@GGG-KILLER
Copy link
Contributor Author

This will probably fix #170
any news @ZEisinger / @GGG-KILLER ?

As I've said before, at this point I'm just waiting for the maintainers.
I don't see a way to test this code without some heavy refactoring of the entire code so I can't add any tests for it.

That said, your issue in #170 is actually the issue I reported in #133 and can be fixed by using the full version: 6.0.100-preview.1.21103.13.

src/setup-dotnet.ts Outdated Show resolved Hide resolved
@vsafonkin
Copy link

vsafonkin commented Feb 19, 2021

@GGG-KILLER , could you please update your branch from main to get updated installer scripts?
Or you can run update for your branch:

npm run update-installers

@GGG-KILLER
Copy link
Contributor Author

@GGG-KILLER , could you please update your branch from main to get updated installer scripts?
Or you can run update for your branch:

npm run update-installers

I've updated my branch from main but it seems it didn't go as I expected. I think I'll restart from main and redo my changes.

@GGG-KILLER
Copy link
Contributor Author

GGG-KILLER commented Feb 19, 2021

@GGG-KILLER , could you please update your branch from main to get updated installer scripts?
Or you can run update for your branch:

npm run update-installers

I've fixed my branch and ran npm run update-installers however it seems the tests are still failing.
The failures seem to be unrelated to my change though.

I've also checked the redirects and I haven't seen an HTTPS to HTTP redirect. It seems to be a bug with @actions/http-client:

PS setup-dotnet> redirects https://dot.net/v1/dotnet-install.sh
01. 301 https://dot.net/v1/dotnet-install.sh
02. 302 https://dotnet.microsoft.com/download/dotnet-core/scripts/v1/dotnet-install.sh
03. 200 https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.sh
PS setup-dotnet> redirects https://dot.net/v1/dotnet-install.ps1
01. 301 https://dot.net/v1/dotnet-install.ps1
02. 302 https://dotnet.microsoft.com/download/dotnet-core/scripts/v1/dotnet-install.ps1
03. 200 https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1

The redirect emitted on https://dotnet.microsoft.com/download/dotnet-core/scripts/v1/dotnet-install.sh return a relative Location header: /download/dotnet/scripts/v1/dotnet-install.sh (same for the powershell script).

The issue is on the following code that doesn't account for relative URLs in the Location header: https://github.com/actions/http-client/blob/edadda14b09058389cd47f9790e19b97883624c1/index.ts#L386-L390

@GGG-KILLER
Copy link
Contributor Author

I've opened actions/http-client#38 to track the issue about the install script tests failing.

@ZEisinger
Copy link
Contributor

FYI, I am no longer a maintainer of this repo. My personal recommendation is that the -Channel option be used by this purpose. The intention of moving towards using the dotnet-install scripts was to not repeat implementing features like prelease and ideally removing any need to parse the DotNetCoreIndexUrl.

https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-install-script#options

@GGG-KILLER
Copy link
Contributor Author

FYI, I am no longer a maintainer of this repo. My personal recommendation is that the -Channel option be used by this purpose. The intention of moving towards using the dotnet-install scripts was to not repeat implementing features like prelease and ideally removing any need to parse the DotNetCoreIndexUrl.

https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-install-script#options

The issue with the -Channel approach is that it doesn't support things like 5.x or 3.x, which I assume some people will want to be able to use when using include-prerelease.

@vsafonkin
Copy link

@ZEisinger , thank you for your help! As I see, -Channel option requires exact branch name for release. In order to use it for version with wildcard we should pull some metadata to get exact release name, in any case. In my opinion, since we already pull metadata for releases, it is easier to get pre-release version from this data and pass it to installer script. I suppose it will be possible to get rid of parsing of any external metadata only when the installer scripts arguments will be support the wildcards for versions.

@GGG-KILLER
Copy link
Contributor Author

So @vsafonkin, given that the build failures aren't related to my changes, can we merge this?

@vsafonkin
Copy link

cc: @maxim-lobanov

@GGG-KILLER
Copy link
Contributor Author

@maxim-lobanov could we get this approved? The CI failure isn't related to my change and it's been 19 days...

@GGG-KILLER
Copy link
Contributor Author

Merge conflicts fixed and all checks passing.

@maxim-lobanov maxim-lobanov merged commit 7b20097 into actions:main May 5, 2021
@maxim-lobanov
Copy link
Contributor

@GGG-KILLER , thank you for contribution and I am sorry that it took so much time to accept this PR.
We are going to release the new version in a few days

@GGG-KILLER
Copy link
Contributor Author

@GGG-KILLER , thank you for contribution and I am sorry that it took so much time to accept this PR.
We are going to release the new version in a few days

No problem, the important thing is that it did get merged!

I'll be waiting for the release. If you can drop a comment in here notifying us when it is out it'd be nice so people watching this can know as well

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

Successfully merging this pull request may close these issues.

Add an option to pass includePrerelease to semver
8 participants