Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Touch-ups to master#2084

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
neuecc merged 5 commits intomasterfrommasterFixes
Dec 9, 2024
Merged

Touch-ups to master#2084

neuecc merged 5 commits intomasterfrommasterFixes
Dec 9, 2024

Conversation

@AArnott
Copy link
Collaborator

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

@AArnottAArnott added this to thev3.0 milestoneDec 6, 2024
@AArnottAArnott requested a review fromneueccDecember 6, 2024 13:53
@AArnott
Copy link
CollaboratorAuthor

Questions for you,@neuecc:

  1. Why do you builddebug 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 aton of code to update package.json and yet nbgv can do it automatically for you.

@AArnottAArnott marked this pull request as ready for reviewDecember 6, 2024 15:09
@AArnott
Copy link
CollaboratorAuthor

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 usenbgv tag to create a tag on any commit. It will figure out the version number. You can also usenbgv get-version -v NuGetPackageVersion to determine what the nuget package version will be.
  2. You can set the package.json version withnpm version ${VERSION} --no-git-tag-version --allow-same-version, where you get the${VERSION} fromnbgv get-version -v NpmPackageVersion.

@neuecc
Copy link
Member

neuecc commentedDec 9, 2024
edited
Loading

@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
CollaboratorAuthor

Regarding nuget package build, if you see no value in pushing CI packages to a feed, that's fine. But I strongly encouragebuilding 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
CollaboratorAuthor

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

@neueccneuecc 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.

AArnott reacted with thumbs up emoji
@neueccneuecc merged commit10d9d9b intomasterDec 9, 2024
3 checks passed
@neueccneuecc deleted the masterFixes branchDecember 9, 2024 03:07
@AArnott
Copy link
CollaboratorAuthor

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@neueccneueccneuecc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@AArnott@neuecc

[8]ページ先頭

©2009-2025 Movatter.jp