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

Use module-aware mode instead of GOPATH mode #238

Merged
merged 3 commits into from
Nov 16, 2021
Merged

Conversation

markdascher
Copy link
Contributor

@markdascher markdascher commented Nov 12, 2021

GOPATH development mode will be removed from Go any day now. This PR makes the necessary adjustments to use module-aware mode instead. This also eliminates the need for govendor, which is obsolete.

The majority of changes come from running the commands below. NOTE: go mod init gets confused when vendor.json lists transitive dependency revisions, since they use the commit ID of the parent. Specifically, spew and difflib both used d77da356e56a7428ad25149ca77381849a6a5232 from testify. To make it do the right thing, I looked up the actual commit IDs of both packages and replaced them in vendor.json first.

go mod init github.com/papertrail/remote_syslog2
go mod tidy
rm -rf vendor
go mod vendor

Afterwards, I ran PACKAGE_VERSION=0.20 make to confirm that everything still tests and builds successfully. Also confirmed that the resulting vendor folder was identical, accounting for some minor differences between govendor and go mod vendor:

We could also consider dropping the vendor folder entirely, but it doesn't hurt to keep it for now. (And especially for this PR, makes it more obvious that nothing has changed.)

@markdascher markdascher self-assigned this Nov 12, 2021
Makefile Outdated
type gox >/dev/null 2>&1 || { \
echo "\033[1;33mGox is not installed. See https://github.com/mitchellh/gox\033[m"; \
echo "$$ go get github.com/mitchellh/gox"; \
echo "$$ go install github.com/mitchellh/gox@latest"; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I wonder if it would be safer to pin this to a specific version? @tillberg what do you think?

Choose a reason for hiding this comment

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

Is this command run in CI? If so, then this should use a cryptographic hash for the version reference. Otherwise, I think I'm hesitant to weigh in one way or the other :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not part of CI, but it is part of the manual build/release process. I like the idea of making the version explicit. Added tools.go in cb74f9f based on this technique, which seems to be as good as it gets while we wait for golang/go#48429.

This could've been messy if gox shared any dependencies with remote_syslog2 itself, but I confirmed there weren't any shared dependencies.

@@ -1,6 +1,6 @@
# remote_syslog2

[![Download remote_syslog2](http://papertrail.github.io/remote_syslog2/images/download.png)][releases]
[![Download remote_syslog2](https://papertrail.github.io/remote_syslog2/images/download.png)][releases]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed all http:// links to https:// while I was in here. Confirmed that the new links all work.

README.md Outdated
Comment on lines 354 to 355
go get github.com/kardianos/govendor
go get github.com/mitchellh/gox
go get github.com/papertrail/remote_syslog2
go install github.com/mitchellh/gox@latest
git clone git@github.com:papertrail/remote_syslog2.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://golang.org/doc/go-get-install-deprecation
golang/go#31529 (comment) discusses using git clone instead of go get for local development

@@ -66,7 +66,7 @@ func TestRawConfig(t *testing.T) {
assert.Equal(c.Facility, fac)
assert.NotEqual(c.Hostname, "")
assert.Equal(c.Poll, false)
assert.Equal(c.RootCAs, papertrail.RootCA())
assert.Equal(c.RootCAs.Subjects(), papertrail.RootCA().Subjects())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@icoleslaw icoleslaw left a comment

Choose a reason for hiding this comment

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

LGTM

@markdascher
Copy link
Contributor Author

I was able to produce the same exact executables using by running make before and after this PR's changes (with a bit of wrangling), so I think we're in good shape here.

@markdascher markdascher merged commit 4b0290c into master Nov 16, 2021
@markdascher markdascher deleted the module-aware branch November 16, 2021 15:42
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