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

Upgrade goreleaser to v2 #23703

Closed
wants to merge 15 commits into from
Closed

Upgrade goreleaser to v2 #23703

wants to merge 15 commits into from

Conversation

sgress454
Copy link
Contributor

Checklist for submitter

This PR updates Goreleaser to version 2. Updates include:

  • Upgrading the goreleaser dependency to v2
  • Updating CLI args in workflows in accordance with the upgrade guide
  • Updating goreleaser YML files, also in accordance with the upgrade guide
  • Small code update needed due to changes in the underlying nfpm package (the ExpandContentGlobs method was replaced by PrepareForPackager). The fact that this change wasn't considered a breaking change makes me think that we may be relying on a private API here.

I was able to run the Docker Publish workflow successfully on this branch. The Gorelease Orbit workflow runs successfully for MacOS, Linux and Linux-arm64, but fails on Windows due to it not knowing how to parse the tag name. I'm pushing a small patch to this branch that should allow tags like v1.36.0-1-some-notes to allow prerelease tags with notes. I wouldn't merge this before seeing that pass.

args: release --rm-dist -f .goreleaser.yml
version: "~> 2"
args: release --clean -f .goreleaser.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

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


- name: Run GoReleaser
run: go run github.com/goreleaser/goreleaser@56c9d09a1b925e2549631c6d180b0a1c2ebfac82 release --debug --rm-dist --skip-publish -f orbit/goreleaser-macos.yml # v1.20.0
run: go run github.com/goreleaser/goreleaser/v2@606c0e724fe9b980cd01090d08cbebff63cd0f72 release --verbose --clean --skip=publish -f orbit/goreleaser-macos.yml # v1.20.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines -139 to +141
name_template: "{{ .Tag }}-untagged"
version_template: "{{ .Tag }}-untagged"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines -142 to +144
skip: true
disable: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgress454 sgress454 marked this pull request as draft November 11, 2024 23:25
@@ -5,5 +5,5 @@ import "github.com/goreleaser/nfpm/v2/deb"
// BuildDeb builds a .deb package
// Note: this function is not safe for concurrent use
func BuildDeb(opt Options) (string, error) {
return buildNFPM(opt, deb.Default)
return buildNFPM(opt, deb.Default, "deb")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed as the replacement for ExpandContentGlobs (i.e. PrepareForPackager) expects the name of the packager to be passed in, and I didn't see a way to get it from the packager var itself.

@@ -192,7 +193,7 @@ func buildNFPM(opt Options, pkger nfpm.Packager) (string, error) {
contents = append(contents, (&files.Content{
Destination: emptyFolder,
Type: "dir",
}).WithFileInfoDefaults())
}).WithFileInfoDefaults(0o0, time.Now()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WithFileInfoDefaults now expects a umask and an mtime to use if the file info doesn't include permissions or timestamp data. Since we weren't using this before, I'm providing a umask that's a no-op and a fallback mtime of "now" since that's what it did before. Since these are new folders I don't think this should cause any problems (the umask is intended to provide sane permissions in cases where a user may have changed perms on a file locally before adding it to a package).

@@ -403,7 +403,7 @@ func SanitizeVersion(version string) ([]string, error) {
}
if len(vParts) == 3 && strings.Contains(vParts[2], "-") {
parts := strings.SplitN(vParts[2], "-", 2)
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
if len(parts) < 2 || parts[0] == "" || parts[1] == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intended to allow versions like orbit-v.1.2.3-1-some-notes-about-the-prerelease, where previously the notes part would have caused an error.

Comment on lines -168 to +169
Destination: "/",
Destination: "",
Copy link
Contributor Author

@sgress454 sgress454 Nov 12, 2024

Choose a reason for hiding this comment

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

Without this, we now get content collision errors as the behavior has changed for destinations ending in slashes. See goreleaser/nfpm#671 and goreleaser/nfpm#576.

Copy link
Member

@mna mna left a comment

Choose a reason for hiding this comment

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

Didn't realize this was still a draft when I got the notification, but oh well might as well post my review. Changes LGTM but I'm not super familiar with the goreleaser build, might want to also wait for a review by someone who knows it better (I think Lucas might?).

github.com/go-ini/ini v1.67.0
github.com/go-kit/kit v0.12.0
github.com/go-kit/log v0.2.1
github.com/go-ole/go-ole v1.2.6
github.com/go-sql-driver/mysql v1.7.1
github.com/go-sql-driver/mysql v1.8.1
Copy link
Member

Choose a reason for hiding this comment

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

Just calling this out as there are some important version upgrades in this PR (scanning those quickly, the mysql driver might be the big one), we should make sure to do some smoke tests in QA once this lands in addition to automated tests.

@@ -19,7 +20,7 @@ import (
"github.com/rs/zerolog/log"
)

func buildNFPM(opt Options, pkger nfpm.Packager) (string, error) {
func buildNFPM(opt Options, pkger nfpm.Packager, packagerName string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit, but would it be clearer as to what to pass with packagerType or packagerExt?

@sgress454
Copy link
Contributor Author

@mna

Didn't realize this was still a draft when I got the notification, but oh well might as well post my review. Changes LGTM but I'm not super familiar with the goreleaser build, might want to also wait for a review by someone who knows it better (I think Lucas might?).

np I moved it back to draft when I saw that tests were failing. But re: the mysql and other dependency upgrades, they don't seem to be necessarily related to goreleaser and I think they're likely related to the test failures considering this PR doesn't update any functional code. So I'm going to back those out and try to update only goreleaser-related dependencies.

@sgress454
Copy link
Contributor Author

sgress454 commented Nov 12, 2024

Closing this in favor of #23748 which doesn't require code changes.

@sgress454 sgress454 closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants