search

martincostello / update-dotnet-sdk

A GitHub Action that updates the .NET SDK
Apache License 2.0
18 stars 1 forks source link

Pinning to a .NET feature band #889

Open dorssel opened 6 months ago

dorssel commented 6 months ago

Describe the bug

I am trying to pin to a certain feature branch: 8.0.2xx. I do so by specifying channel: '8.0.2xx'. The logs give me:

Run martincostello/update-dotnet-sdk@v3
Current .NET SDK version is 8.0.205
Current .NET runtime version is 8.0.5
Latest .NET SDK version for channel '8.0.2xx' is 8.0.300 (runtime version 8.0.5)
Updating .NET SDK version in '/home/runner/work/usbipd-win/usbipd-win/global.json' to 8.0.300...
Updated SDK version in '/home/runner/work/usbipd-win/usbipd-win/global.json' to 8.0.300
Updated git user name to 'github-actions[bot]'
Updated git user email to 'github-actions[bot]@users.noreply.github.com'
The update-dotnet-sdk-8.0.300 branch already exists

I expect the updater to fixate on 8.0.2xx. 8.0.205 is the latest for 8.0.2xx. But, for example, if current version would be 8.0.204, then I expect an update to 8.0.205.

This URL also gives 8.0.205 as result: https://aka.ms/dotnet/8.0.2xx/sdk-productVersion.txt

But it appears this is not the URL from where you determine the latest version?

Steps To reproduce

See: https://github.com/dorssel/usbipd-win/pull/954/files

Additional context

Maybe I also need a quality setting, as in your code (https://github.com/martincostello/update-dotnet-sdk/blob/main/src/DotNetSdkUpdater.ts#L523), the sdk-productversion is retrieved only if both channel and quality are set. But I wouldn't know which quality to set for the correct results. Neither daily, signed, validated, nor preview work...

Or maybe I need some other configuration to get this to work?

martincostello commented 6 months ago

This isn't a bug, it's by design.

According to the release notes JSON, which are the canonical source-of-truth as far as I'm aware (the dot.net website uses it to offer downloads), the latest SDK release is 8.0.300, so that's what you get.

The channel and quality settings are only used to get daily builds - you won't get "official" builds of .NET from using those settings 99% of the time, unless the most recent build happened to be what will be the official one. When releases have security fixes, they're built separately to the public stuff in internal branches, so often there's a disconnect between what's visible in the nightly builds in public compared to what the official build actually is.

That said, it did occur to me this week that the action doesn't currently support people wanting to stay on a specific feature band, like say 8.0.1xx or 8.0.2xx. That's something I can look to add at some point when I have the time (unless you'd like to have a stab at contributing the support to do so).

dorssel commented 6 months ago

Thanks for your quick and thorough response.

Yes, that explains it. And I certainly would like to keep it as a feature request. I'll see if I can brush up on my TS skills, they are a bit rusty (pun intended...). In the mean time, do you know of any official documents on URLs like https://aka.ms/dotnet/8.0.2xx/sdk-productVersion.txt (which seems to be what I am after)?

martincostello commented 6 months ago

do you know of any official documents on URLs like...

I'm afraid not. The support for daily builds was essentially reverse-engineered by me looking at this table in the dotnet/installer repository and the .NET install scripts (which is what actions/setup-dotnet uses).

dorssel commented 6 months ago

Would an additional boolean setting pinFeatureBand be OK to you? Then either the combination channel: A.B.Cxx + pinFeatureBand: true triggers this function, or else the current global.json SDK version + pinFeatureBand: true.

martincostello commented 6 months ago

I haven't fully thought through what the best way to do it would be yet. I was thinking maybe something like how actions/setup-dotnet works and if the channel was of the format A.B.Cxx the logic here would instead find all the SDK versions in the notes that matched the pattern, and then take the latest one of what it finds, rather than just taking the value of latest-sdk.

dorssel commented 6 months ago

That sounds reasonable, but would be a breaking change. I'm OK with that, but are you?

dorssel commented 6 months ago

And additionally. It would be nice if the feature band could be pinned simply based on a single setting while keeping global.json as the single source of truth for the actual channel + feature band. In fact, we could even use a "rollForward: latestPatch" setting in global.json to trigger this...

martincostello commented 6 months ago

What's your reasoning on how that we be a breaking change?

we could even use a "rollForward: latestPatch" setting

I like this idea, doing what the user is already indicating, but I'm not sure if the semantics between intent for the runtime and what SDK to update to necessarily match.

For example here I set latestMajor which is basically to say "you can use a newer SDK if this version isn't installed", but that doesn't really mean anything to this action because it's not intended to upgrade you across major versions, just within the channel you're already in.

Maybe a new setting that's a similar concept could be added to the action? featureBand: "latest|latestPatch". Even if there were only two options, it could be more flexible if there needed to be other options in the future.

dorssel commented 6 months ago

Yes, I like that. And I guess here you mean that latest and latestPatch just do the same? (if so, we could consider only "latestPatch", as "latest" is really legacy).

On the other hand (also for easy documentation), maybe we should just support exactly the same options as global.json's "rollForward", and indeed update to the maximum available SDK version that rollForward would be willing to use?

martincostello commented 6 months ago

What I intended was:

Maybe the naming just needs to be better.

Whatever the option is, it shouldn't be a breaking change and the behaviour should remain as it is today - it should be opt-in for those who want it (like yourself) as it introduces the burden of needing to manually keep on top of your feature band, as otherwise it'll go stale. 8.0.2xx will eventually not get any new versions again, even while 8.0 is supported, and will get superseded by newer feature bands - you can see this with .NET 6's SDK versions: this month there was a new 6.0.1xx and a 6.0.4xx, but no new updates for 6.0.2xx or 6.0.3xx. The user would need to manually roll over to a future band if they don't stay on A.B.1xx or become stagnant and undermine the benefits of using the action in the first place.

I don't think supporting major makes any sense, as it'll probably just break your code immediately when .NET 9 releases (because it won't update any packages, TFMs, won't have the right runtime installed to run your tests, you might not want it because it's not an LTS...). I have a completely different project for that 😄: martincostello/dotnet-bumper

dorssel commented 6 months ago

And what I intended was: see global.json spec: https://learn.microsoft.com/en-us/dotnet/core/tools/global-json#rollforward

So, for example, if global.json (or your channel) specifies 8.0 and you have rollForward (let's call it the same in your config) as latestMinor. Then global.json would mean: the highest available SDK that matches 8.x that is currently installed. And for us it would mean: the highest available SDK in the releases.txt that would still match 8.x.

Now it is fully configurable. I would use rollForward: 'latestPatch', others may use whatever. (and in this sense, patch would be the same as latestPatch). And yes, I was confusing that with your latest|latestPatch, which I read as patch|latestPatch.

Your latest is called latestFeature in global.json terms...

In short: I would like to have all the stuff that global.json could do for matching installed SDKs available in this action to mean matching available SDKs. That is also easy to explain. And maybe we should just use the options already configured in global.json itself. After all, if that says latestMajor (which is tricky, I agree, I would personally never use that), then obviously the author intends that the code requires any SDK >= the current mentioned version.

martincostello commented 6 months ago

We could maybe use the same terms, but omit support for some of them (i.e. the major ones). So maybe:

I don't want to go down the slippery slope of supporting major upgrades here, as what people would want them to do wouldn't match what it would actually do in reality, and I have no intention of adding that complexity to this relatively simple TypeScript action (hence my other project).

martincostello commented 6 months ago

latestMinor (which at this point is probably redundant as we haven't had a minor release since .NET Core 3.1)

In fact, that wouldn't be used, because 3.0 was a different channel to 3.1 anyway, so would never be in the same JSON file (assuming they don't change their processes).

So essentially we're left with two modes: one for features (today's behaviour) and one for patches (what you want it to do).

dorssel commented 6 months ago

We could maybe use the same terms, but omit support for some of them (i.e. the major ones). So maybe:

  • latestPatch
  • latestFeature
  • latestMinor (which at this point is probably redundant as we haven't had a minor release since .NET Core 3.1)

Yes, this makes the most sense. Shall we also call it rollForward? And default to current behavior, (i.e. if you don't set the option): latestFeature?

That would mean:

martincostello commented 5 months ago

That sounds good to me.

So:

rollForward:
  description: 'The optional roll-forward behavior for .NET SDK updates. Must be one of: latestFeature, latestPatch.'
  required: false
  default: 'latestFeature'