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

feat: support global.json rollForward latest variants #481

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

the-avid-engineer
Copy link

@the-avid-engineer the-avid-engineer commented Nov 15, 2023

Description:
This PR adds support for all of the latest* variants of rollForward option in the global.json file

  • latestMajor
  • latestMinor
  • latestFeature
  • latestPatch

Related issue:
#448

Check list:

  • Mark if documentation changes are required. <- I'm not sure?
  • Mark if tests were added or updated to cover the changes.

I'm not sure if it makes sense to add the same kind of test for latestMajor - it would break every time there's a new major release. If it weren't limited to regular expression checks, the test could provide an EOL sdk version such as 3.1.426, and the test could make sure that the resolved major version is greater than 3, but I'm not sure how to go about that.

@the-avid-engineer the-avid-engineer requested a review from a team as a code owner November 15, 2023 20:06
the only way this is valid is if rollForward is `latestMajor`, which is the only value that the global.json spec says doesn't require a version to be specified
@the-avid-engineer
Copy link
Author

the-avid-engineer commented Nov 15, 2023

One concern I have is that the README states that using the partial version numbers will include prereleases (by default?).. which makes me think it should also support the allowPrerelease part of global.json as well.. I suppose it should force the quality to GA if it's false, but..

  1. What's the best way to go about that? Parse the global.json file twice (once for version, once for quality)? Or refactor the code so that it's only parsed once and passed around as needed

  2. That would force quality for all versions, not just the one from global.json :/ This doesn't affect me, but there might be people using multiple versions, including one from global.json?

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

n0099 commented Mar 23, 2024

Also addressed #397 (comment)

@js6pak
Copy link

js6pak commented Mar 23, 2024

Keep in mind because of the awful way github actions are distributed you have to run npm run build (you should run format and lint aswell) before every commit, which you didn't since the initial one.

3089bb1 is pointless because the if (globalJson.sdk && globalJson.sdk.version) { check above rules out empty/null version anyway.

Also the CI is failing which you can't see because the workflow is only enabled for pull requests and no one approved it for this one yet. In my opinion you should just disable the branch filtering here, it won't hurt to always run the check.

I've pushed out actually working code here: https://github.com/js6pak/setup-dotnet/commits/feat/support-rollForward/

Copy link

@Moazzem-Hossain-pixel Moazzem-Hossain-pixel left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions.

Copy link

@Moazzem-Hossain-pixel Moazzem-Hossain-pixel left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions.

@AlanRynne
Copy link

This would be useful to us too at Speckle! ♥️ 🙌🏼

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.

5 participants