Logrus import should be lowercase

[pokstad]

#For stories, please include the information below:

User Statement:

As a developer who wants to import this library into other projects, the improper capitalization of the logrus import path is problematic.

Details:

The logrus package is no longer capitalized the way it appears vendored in this project. It was previously capitalized, but now it is lowercase to follow Go best practices. When two different dependencies import the logrus package with both spellings, it causes a conflict:

tags.go:26:2: case-insensitive import collision:

See this related issue for vgo: golang/go#26208

Acceptance Criteria:

All occurrences of Sirupsen in import paths must be replaced with sirupsen. Additionally, the vendored logrus path must be changed to reflect this new spelling.

#For bug reports, please include the information below:

VIC version:

master commit ID 9ad53ee7f7e4beb3897e0e1d4a190f07a21b4280

Deployment details:

N/A

Steps to reproduce:

N/A

Actual behavior:

N/A

Expected behavior:

N/A

Logs:

N/A

Additional details as necessary:

N/A

[banks]

To add some data as well as an up-vote. The Author of logrus admits the renaming was probably a mistake but describes that "Every library should use lower-case to avoid this problem."

Source: https://github.com/sirupsen/logrus#case-sensitivity

In our case we import this package into hashicorp/go-discover along with many other tools that also depend on logrus (all lower case). We end up with both cases in go.mod file there and in all downstream projects (e.g. hashicorp/consul). Despite go mod handling that in some cases (go-discover and consul still build currently) we can no longer import certain other dependencies into Consul because of the case conflict caused by (only) this dependency. I don't fully understand that yet since we can build with existing mixed case transitive deps but adding new dependencies seems to break the build. We'll work that out, but it would be awesome if we could just eliminate this problem for others too here.

[banks]

So it seems that all of the following vendored dependencies still have upper case Sirupsen. Many of them appear to be very old versions of those libraries (maybe because of this issue?).

$ rg -l 'Sirupsen' | rg '^vendor/[^/]+/[^/]+/[^/]+' -o | sort | uniq
vendor/github.com/Azure/go-ansiterm
vendor/github.com/Microsoft/hcsshim
vendor/github.com/Sirupsen/logrus
vendor/github.com/docker/distribution
vendor/github.com/docker/docker
vendor/github.com/docker/go-connections
vendor/github.com/docker/go-events
vendor/github.com/docker/libnetwork
vendor/github.com/docker/swarmkit
vendor/github.com/opencontainers/runc
vendor/gopkg.in/gemnasium/logrus-airbrake-hook.v2

They would all need to be updated to use the lower case version for this issue to be fixed. If that isn't practical, an alternative would be to modify the files in the vendor directory to make them lower case and do this in a repeatable way each time dependencies are vendored. It looks like you are using the makefile as the workflow to run gvt for vendoring so replacing the imports in the make target after vendor restore would be possible if ugly way to solve this.

I understand that's likely not a high priority, but as a library that others have to import, this issue may affect a lot of people who also import other more modern libraries that have standardised on lower case logrus.

For now, we are going to maintain a fork for HashiCorp projects to use that just does the find/replace mentioned above but we'd love to get rid of it once a better solution is found here!

Thanks for the hard work!

[johandry]

This is also happening at ./pkg/vsphere/tags/tags.go:26:2
Please, could you update the package name?

[bhyh]

Any update on this? Would like to use https://github.com/vmware/vic/tree/master/pkg/dio in a project, but the Sirupsen/logrus import issue makes this unwieldy.

[LorbusChris]

@stuclem this issue has been open for more than a year now, is rather annoying and should be an easy fix. I've created another PR at #8611. Please take another look.

[LorbusChris]

Unfortunately make test does not run locally (not on master, and not on #8611 branch) as some of the referenced files don't exist anymore, e.g. lib/apiservers/portlayer/client/port_layer_client.go

All dependencies have already updated their imports to the lower-case variant - after all the issue is more than 2.5 years old: sirupsen/logrus#543

It is also unclear to me how the vendored deps should be updated.

Please fix this asap.

[stuclem]

@stuclem this issue has been open for more than a year now, is rather annoying and should be an easy fix. I've created another PR at #8611. Please take another look.

@renmaosheng can you please take a look at this for @LorbusChris? Thanks!

[LorbusChris]

@stuclem @renmaosheng friendly ping. Any news on this?

[stuclem]

Hi @LorbusChris, I write the VIC documentation, so I'm afraid that I don't have any influence over the prioritization of engineering issues. This is @renmaosheng 's call.

[LorbusChris]

Note for followers of openshift/installer#2745:
This repo is not a dependency of https://github.com/terraform-providers/terraform-provider-vsphere anymore (as of commit hashicorp/terraform-provider-vsphere@05cd12c),
so this issue is not required for that effort anymore.