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 Go modules #168

Merged
merged 5 commits into from
Oct 23, 2019
Merged

Use Go modules #168

merged 5 commits into from
Oct 23, 2019

Conversation

nbutton23
Copy link
Contributor

Simply ran go mod init github.com/pagerduty/go-pagerduty

Converted PagerDuty/go-pagerduty to match lowercased package nameing standards
https://blog.golang.org/package-names

Updated the github.com/Sirupsen/logrus import to current lowercase name

nbutton23 and others added 3 commits September 10, 2019 09:20
Simply ran `go mod init github.com/pagerduty/go-pagerduty`

Converted `PagerDuty/go-pagerduty` to match lowercased package nameing standards
https://blog.golang.org/package-names

Updated the `github.com/Sirupsen/logrus` import to current lowercase name
@nbutton23
Copy link
Contributor Author

@stmcallister Did you get a chance to look at this?

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

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

Because Modules are still experimental I'd personally like to avoid adopting them until they are more stable.

@nbutton23
Copy link
Contributor Author

Modules are used by basically every opensource go project. while its still beta (last i read they are targeting release in 1.14) its the most supported package manager out there for go. Little has changed in the actual API of modules in the last few releases, so i would say is stable.

@stmcallister
Copy link
Contributor

I'm leaning toward @nbutton23 on this one. I'll take a look at this tomorrow.

@stmcallister
Copy link
Contributor

@nbutton23 -- We're still discussing this a bit internally. If I understand it correctly, going to modules will require all existing projects using this library to be moved out of the $GOPATH, which means everyone's projects will break once we release. Is there a smoother way to make this transition?

@nbutton23
Copy link
Contributor Author

@stmcallister It shouldn't break anything. Right now by default GO111MODULE is set to auto. If they are not using mods then nothing will change.

enables module-mode if any go.mod is found, even inside GOPATH. (Prior to Go 1.13, GO111MODULE=auto would never enable module-mode inside GOPATH).

https://github.com/golang/go/wiki/Modules

@stmcallister
Copy link
Contributor

@nbutton23 Except that it broke for me. 😄

$ make build
go get ./...
go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
    ignoring go.mod;
    see 'go help modules'
command/addon_install.go:6:2: case-insensitive import collision: "github.com/pagerduty/go-pagerduty" and "github.com/PagerDuty/go-pagerduty"
make: *** [build] Error 1

I'm guessing this is because the package name changed from PagerDuty to pagerduty?

@nbutton23
Copy link
Contributor Author

Ah yeah thats not the module is self. Its the change made to match naming standards. I can pull that out. . .

@stmcallister
Copy link
Contributor

@nbutton23 Yeah, I see what you're saying. However, if changing the package name at this point is going to break existing apps and negatively affect current users, we want to avoid making that change. If you go ahead and take that out, the rest of the PR looks good.

@nbutton23
Copy link
Contributor Author

@stmcallister Done!

@stmcallister
Copy link
Contributor

@nbutton23 Awesome! Thanks! LGTM.

@stmcallister stmcallister dismissed theckman’s stale review October 23, 2019 15:52

@theckman Thanks for expressing your concern, but we've looked at this internally and have decided this is the direction we want to go with the library.

Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

Using modules is the direction we're hoping to take the library. Also, thank you for fixing the logrus bug.

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