-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(github-actions): Support actions/setup-dotnet with:dotnet-version #37369
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
base: main
Are you sure you want to change the base?
feat(github-actions): Support actions/setup-dotnet with:dotnet-version #37369
Conversation
6282ca5
to
81406a7
Compare
depName: action, | ||
packageName: packageName ?? `actions/${action}-versions`, | ||
versioning, | ||
extractVersion: '^(?<version>\\d+\\.\\d+\\.\\d+)(-\\d+)?$', // Actions release tags are like 1.24.1-13667719799 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be correct, as it doesn't look like it would work for values with an x
in.
type: 'dotnet-sdk-floating-range'; | ||
major: number; | ||
minor?: number | 'x'; | ||
patch?: number | 'x'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example on line 16 does not match the type declared here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True - it's mainly being used as a way to round-trip the string into its parts and back again for the A.B.x
case when floating
is minor
.
Line 16 will be 8
0
400
in memory for the floating: 'patch'
case.
c061318
to
fcf411a
Compare
Add support for extracting `dotnet-version` from `actions/setup-dotnet` in the GitHub Actions manager.
Add some more test cases.
Fix typo in comment. Co-authored-by: Anne Stellingwerf <[email protected]>
Fix typo in comment.
fcf411a
to
94d5cda
Compare
Resolve ESLint warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually don't review draft PR's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for extracting and updating dotnet-version
values from the actions/setup-dotnet
GitHub Action. It implements a new .NET SDK
versioning strategy specifically designed to handle the custom version syntax that actions/setup-dotnet
supports.
- Creates a new
dotnet-sdk
versioning strategy with full support for .NET SDK version formats - Updates the GitHub Actions manager to extract
dotnet-version
values fromactions/setup-dotnet
- Fixes a spelling error in the existing nuget version module
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
lib/modules/versioning/nuget/version.ts | Fixes spelling error in comment |
lib/modules/versioning/dotnet-sdk/*.ts | New dotnet-sdk versioning module with parser, range handling, and comparison logic |
lib/modules/versioning/api.ts | Registers the new dotnet-sdk versioning strategy |
lib/modules/manager/github-actions/extract.ts | Updates GitHub Actions manager to support dotnet-version extraction |
lib/modules/manager/github-actions/extract.spec.ts | Adds comprehensive tests for dotnet-version extraction |
lib/modules/manager/github-actions/readme.md | Updates documentation to mention dotnet support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const xPatch = x.patch ?? 100; | ||
const yPatch = y.patch ?? 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 100 is used as a default patch value without explanation. This should be documented with a comment explaining why 100 is the default patch value for .NET SDK versioning, or extracted to a named constant.
Copilot uses AI. Check for mistakes.
export function versionToString(version: DotnetSdkVersion): string { | ||
let res = `${version.major}`; | ||
|
||
res += `.${version.minor ?? 0}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent default values are used: 0 for minor but 100 for patch. These magic numbers should be extracted to named constants to make the relationship clear and ensure consistency across the codebase.
Copilot uses AI. Check for mistakes.
|
||
res += `.${version.minor ?? 0}`; | ||
|
||
res += `.${version.patch ?? 100}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent default values are used: 0 for minor but 100 for patch. These magic numbers should be extracted to named constants to make the relationship clear and ensure consistency across the codebase.
Copilot uses AI. Check for mistakes.
minor: minor === 'x' ? 0 : minor, | ||
patch: patch === 'x' ? 100 : patch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 100 appears again as the default patch value. This should use the same named constant referenced in the previous comment to maintain consistency.
Copilot uses AI. Check for mistakes.
} else if (r.floating === 'minor') { | ||
return v.major === r.major && (v.minor ?? 0) === (r.minor ?? 0); | ||
} else { | ||
const patch = r.patch === 'x' || !r.patch ? 100 : r.patch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another instance of the magic number 100. This reinforces the need for a named constant to represent the default .NET SDK patch value.
Copilot uses AI. Check for mistakes.
} = groups; | ||
|
||
if (prerelease) { | ||
res.prerelease = groups.prerelease as `${string}*`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion as
${string}`` is incorrect. The prerelease value doesn't necessarily end with '' and this type assertion is misleading. It should be as string
or the type should be corrected.
res.prerelease = groups.prerelease as `${string}*`; | |
res.prerelease = groups.prerelease as string; |
Copilot uses AI. Check for mistakes.
currentValue.split(newlineRegex).forEach((value) => { | ||
if (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The nested forEach loop with conditional push could be refactored using filter and map for better readability: currentValue.split(newlineRegex).filter(Boolean).map(value => ({ ... })).forEach(dep => deps.push(dep))
Copilot uses AI. Check for mistakes.
I'm not 100% on exactly how to approach this, so feedback/guidance sought. Then I can deal with the Copilot review comments and any other feedback. |
Changes
Add support for extracting
dotnet-version
fromactions/setup-dotnet
in the GitHub Actions manager.Context
Renovate does not currently support updating the value specified by
dotnet-version
withactions/setup-dotnet
such as the below:These changes should light-up support based on the custom syntax for the .NET SDK version the action supports (which is not the same syntax
global.json
itself supports).I'm not sure what the best name for the new versioning strategy should be as it seems specific to the
actions/setup-dotnet
action. For now I've called itdotnet-sdk
and based it onnuget
.Examples
Example updates this PR should enable depending on configuration.
Major update for A
Major update for A.x
Major update for A.B
Major update for A.B.x
Major update for A.B.Cxx
Minor update for A.B.Cxx
Minor update for A.B.C
Major update for A.B.C
Pinning for minor A.B.x
Minor update for two A.B.C values
I'm not sure if the last example will work or not - I've updated the manager so that it correctly extracts multiple values, but whether they would be successfully applied to the YAML file is a different question. I couldn't immediately see where to add a test for that scenario.
Documentation
No documentation update is requiredHow I've tested my work
I have verified these changes via:
Code inspection only, orNo unit tests but ran on a real repository, orBoth unit tests + ran on a real repository