polds / logrus-papertrail-hook

Papertrail hook for Logrus - https://github.com/sirupsen/logrus
MIT License
12 stars 8 forks source link

Is changing Sirupsen import capitilzation a breaking change? #8

Closed ExternalReality closed 6 years ago

ExternalReality commented 7 years ago

https://github.com/polds/logrus-papertrail-hook/commit/734dc5acdd79326b99d0aad332221f87621860af

...changes the capitalization of the Sirupsen in the logrus import. Will this cause import collisions downstream?

https://github.com/sirupsen/logrus/issues/451

polds commented 7 years ago

Yeah, it is a breaking change due to Sirupsen changing to sirupsen, As it is now github.com/Sirupsen/logrus is no longer importable as it doesn't exist. Without that change it makes this package non-importable as well.

I'm open to suggestions on how to resolve it, as you can see in that issue many other hooks have had similar discussions.

undiabler commented 7 years ago

Are you fucking kidding me? all my projects is now broken logrus real path now is github.com/sirupsen/logrus @polds why do you submit merge with outdating Sirupsen repo?

@evalphobia if you have errors with case-insensitive import collision as you write here https://github.com/polds/logrus-papertrail-hook/pull/9 with other hooks please fix path in you hooks but dont breake this repo

P/s for all who wants to use outdating repo please read about gopkg and use

import "gopkg.in/polds/logrus-papertrail-hook.v2"

in you projects

polds commented 7 years ago

@undiabler So I guess I'm confused now, in https://github.com/sirupsen/logrus/issues/451 they discussed reverting the naming change and keeping it Sirupsen as #9 addresses. Because the original issue was them changing Sirupsen to sirupsen. So did the revert not actually take place and the proper import is github.com/sirupsen/logrus?

Side note, would you like collaborator access to this repo? Most of my projects that use this project are dying and I haven't had to redeploy recently to see broken import paths.

polds commented 7 years ago

Also to note logrus' readme still shows examples with importing github.com/Sirupsen/logrus.

undiabler commented 7 years ago

@polds yeah, actual import is sirupsen/logrus you can check real path in sirupsen account This situation is really confusing because a lot of repos is still outdating and use old links.

We use papertrail in our company products (15+ private repos) so I can help you with collaborating this repo because it's really very important for me.

I think we should use new path. If somebody wants to use old import they shoud use gopkg and we can write about this in readme and create new tags for gopkg.

undiabler commented 7 years ago

Also there is pull request https://github.com/sirupsen/logrus/pull/517 for changing path in readme, but it isnt merged yet.

undiabler commented 7 years ago

Summary:

So I've reverted last merge until the situation is clarified. Also I've added new major version for using v2/v3 version with gopkg

evalphobia commented 7 years ago

tl;dr

Sorry for confusing. 😅 It's okay to use github.com/sirupsen/logrus, I don't use this package for my project. We hope he move logrus to other group (something like github.com/go-logrus/logrus) in this year and it solves this problem.


At first, the vendoring tool GB had case-sensitive problem and tried to change Sirupsen to sirupsen (https://github.com/sirupsen/logrus/pull/384) But it cause other case-sensitive problem for the developer who use other packages importing github.com/Sirupsen/logrus.

So the change was reverted. Then other logrus helper or hooks followed to revert sirupsen to Sirupsen. (see referenced issues on https://github.com/sirupsen/logrus/issues/451 ) Now All of the code uses logrus/Sirupsen.

git clone (on GitHub) and go get command is not case-sensitive, so we can still use github.com/Sirupsen/logrus. (even github.com/siruPSEN/lOgrUs works!) But the package is saved on the path you called.

# e.g. use siruPSEN/lOgrUs

# download repo
go get github.com/siruPSEN/lOgrUs

# check
tree $GOPATH/src/github.com/siruPSEN
$GOPATH/src/github.com/siruPSEN
`-- lOgrUs
    |-- CHANGELOG.md
    |-- LICENSE
    |-- README.md
    |-- alt_exit.go
...

It's not the matter on one hook and your internal package which you have the right to change it. But if some ~Glide~ user uses this hook and other package using logrus and they use Sirupsen, he/she might got case-insensitive error.

So I thought it's good to standardize it to avoid case-insensitive error for ~Glide~ user. I checked logrus hooks on the official repo list, most of all use Sirupsen except for this hook and logrus-bugsnag. (To be fair, many repos have not been maintained long time so they does not change Sirupsen to sirupsen. But official syslog repo use Sirupsen so I choose to use it.)


Modified: I referred to Glide but it's not only Glide problem.

undiabler commented 7 years ago

Okay, then we can save v3 tag with low case and update master branch

undiabler commented 7 years ago

@evalphobia thank you for explainig