Skip to content

Conversation

@AArnott
Copy link
Collaborator

@AArnott AArnott commented Dec 6, 2024

I plan to add to this PR to re-activate NB.GV as the determiner of version numbers.

@AArnott AArnott added this to the v3.0 milestone Dec 6, 2024
@AArnott AArnott requested a review from neuecc December 6, 2024 13:53
@AArnott
Copy link
Collaborator Author

AArnott commented Dec 6, 2024

Questions for you, @neuecc:

  1. Why do you build debug configuration for PR/CI? Most everyone builds release for that, which is useful because we ship the release config so building it more regularly can help catch release-only flaws.
  2. Why do you not build nuget packages on PR/CI? That similarly delays finding a whole class of bugs that otherwise a pull request could find and block on.

I can dramatically simplify your release workflows. You have a ton of code to update package.json and yet nbgv can do it automatically for you.

@AArnott AArnott marked this pull request as ready for review December 6, 2024 15:09
@AArnott
Copy link
Collaborator Author

AArnott commented Dec 6, 2024

I'm hesitant to rewrite your release workflows, though I think they could be dramatically simpler, because I gather you're reusing these workflows across repos at your company/team, and you may not like having one repo that does things differently.

But I recommend you apply some changes:

  1. Stop taking the tag name from the user at workflow dispatch and leverage NB.GV to determine what it should be. You can use nbgv tag to create a tag on any commit. It will figure out the version number. You can also use nbgv get-version -v NuGetPackageVersion to determine what the nuget package version will be.
  2. You can set the package.json version with npm version ${VERSION} --no-git-tag-version --allow-same-version, where you get the ${VERSION} from nbgv get-version -v NpmPackageVersion.

@neuecc
Copy link
Member

neuecc commented Dec 9, 2024

@AArnott

  1. Yes, I agree that using the release build would be best. Let's do that.

  2. I had been skipping it since package versioning is tied to releases. If it can be automated with NB.GV, that would be good. However, I don't see much value in building NuGet packages themselves (while we could upload them to GitHub packages, I haven't seen many cases where they're effectively utilized due to credentials issues. In this regard, I think Azure Pipelines is superior - I've also used Azure Pipelines when distributing internal packages).

I think your suggestion is good, so I'd like to consider it.

Could you write about the release flow using NB.GV in CONTRIBUTING.md?
Including information about when we need to modify version.json.
Also, I'd appreciate if you could delete all Azure Pipelines-related items, including directories and files.

@AArnott
Copy link
Collaborator Author

AArnott commented Dec 9, 2024

Regarding nuget package build, if you see no value in pushing CI packages to a feed, that's fine. But I strongly encourage building those packages on every build anyway, even if they aren't pushed anywhere, because package build authoring is tricky and can easily break the build if PR builds don't validate them.

I'll make the changes you suggest and enable package build (but not push) on PR/CI.

@AArnott
Copy link
Collaborator Author

AArnott commented Dec 9, 2024

The release flow using NB.GV doesn't much apply, in fact NB.GV itself doesn't much apply, with your workflows written the way they are, which overrides the version information at the time you queue the release build. So if you'd like to consider how I do releases on Github Actions with NB.GV, I think that should be a separate PR.

Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

Thank you, this seems fine for now so I'll merge it.
While I'd like to actively consider proper utilization of NB.GV, there's also a possibility that we might completely remove it.
I'll continue to comprehensively evaluate package uploading options, including hosting on Azure.

By the way, if I may say one thing here - I've noticed that recently there seems to be an overt tendency to push people toward Nerdbank.MessagePack.
I think it would be better to refrain from repeatedly promoting it in other repositories, as that kind of behavior could be seen as trolling.

@neuecc neuecc merged commit 10d9d9b into master Dec 9, 2024
3 checks passed
@neuecc neuecc deleted the masterFixes branch December 9, 2024 03:07
@AArnott
Copy link
Collaborator Author

AArnott commented Dec 9, 2024

While I'd like to actively consider proper utilization of NB.GV, there's also a possibility that we might completely remove it.

Ya, I suspected that might be the direction you're going, which is why I haven't spent time bringing it back to the release pipeline yet. Let me know if you want help bringing it back, but I'm content to let you drive either direction.

I think it would be better to refrain from repeatedly promoting it in other repositories, as that kind of behavior could be seen as trolling.

Thanks for letting me know how you feel about it. I'll refrain from it.

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.

3 participants