vmware-archive / vsphere-storage-for-docker

vSphere Storage for Docker
https://vmware.github.io/vsphere-storage-for-docker
Apache License 2.0
251 stars 95 forks source link

Sirupsen/logrus needs to be renamed to sirupsen/logrus #2000

Open akutz opened 6 years ago

akutz commented 6 years ago

All instances of Sirupsen/logrus in Go code need to be renamed to sirupsen/logrus. Please see this comment for more information about this issue.

akutz commented 6 years ago

Hi @msterin,

I'm not sure if you're still on this project, but I thought I'd reach out. I'm working on porting this driver to CSI, and this issue is a large one.

To make matters worse, I tried creating a PR to handle this issue, but y'all rely on such old versions of many of Docker's packages that several of those no longer exist (I'm not talking about Docker's move to moby/moby), and I don't know what to use in place of them. Your dep manager, gvt, is also very difficult as it handles dependencies per-package instead of per-repo. You have different commits per the same repo. It's weird. I recommend going to golang/dep.

As it is I've had to simply copy your sources from the vmkdops, vmci, and fs packages as I cannot import vmkdops. Doing that enables me to rename Sirupsen to sirupsen, and I'm able to proceed.

Still, the above should be corrected ASAP.

msterin commented 6 years ago

@akutz - I am not longer on the project. Regardless - yes, gvt and all dependency mgmt is ancient (> 1 yr I guess) and are due for a face lift. Still, am not sure I get the issue. Actually, I am sure I do not get it. You are porting to CSI - why does it require logrus package version update ? And if it does not need the update , why does it require rename ? And it it does require rename, what is the issue with the rename ? It feels that the upgrade of logrus is required (I am not sure why) and this is hitting old dependencies which are in turn use the old version of logrus and need to be patched... is it correct ?

Meanwhile, I am bumping up the priority and tagging a blocker to make sure the current team takes a look asap.

@shuklanirdesh82 - please bring it up for triage.

akutz commented 6 years ago

Hi @msterin,

I recommend reading the logrus author's comment on the whole debacle. He changed his GitHub user name from Sirupsen to sirupsen. That means the import path changed from github.com/Sirupsen/logrus to github.com/sirupsen/logrus. Not only are many filesystems not case-sensitive, but Go itself will refuse to load two package paths that differ only by case.

Most new Go code written in the last year or uses sirupsen, which makes it incompatible with older Go code. I first discovered this a few months back when I pulled in an updated version of a dependency that had made the switch. It wouldn't load into my project. I had to update the case in all my sources and dependency sources.

akutz commented 6 years ago

Hi @msterin,

It feels that the upgrade of logrus is required (I am not sure why)

Because you don't control what dependencies use, and sometimes you need to update a dependency. If they've made the switch, you're hosed.

msterin commented 6 years ago

Ah, got it, thanks @akutz . Yeah, this sucks. I wish golang simply added version to import statement :-( . I never understood why they have not.

At any rate - I agree it's a must to fix ASAP as it blocks bringing in any fresh dependencies and doing any meaningful work (e.g. port to CSI :-). Let's see what the current team says. //CC @tusharnt

akutz commented 6 years ago

Hi @msterin,

I recommend moving to golang/dep myself. Also, FWIW, I did spend time trying to fix all this via a PR. Here's where I landed:

Here's a gist with the updated manifest and the output when I ran gvt restore.

akutz commented 6 years ago

FWIW, I just ran dep init to convert the updated manifest to use golang/dep, and it worked.

Looks like nothing in your project actually still uses those three deprecated projects, but since gvt does not know how to reconcile that (dep does), gvt restore fails:

$ grep -R '"github.com/docker/docker/pkg/promise"' *
grep: warning: vendor/github.com/coreos/etcd/cmd/etcd: recursive directory loop

$ grep -R '"github.com/docker/docker/utils"' *
grep: warning: vendor/github.com/coreos/etcd/cmd/etcd: recursive directory loop

$ grep -R '"github.com/docker/docker/pkg/random"' *
grep: warning: vendor/github.com/coreos/etcd/cmd/etcd: recursive directory loop
shuklanirdesh82 commented 6 years ago

@shuklanirdesh82 - please bring it up for triage.

Sure, Thanks @akutz !

akutz commented 6 years ago

No problem. Happy to help :)

govint commented 6 years ago

@akutz (tangential maybe) Docker doesn't have any support for the CSI at present are you then creating some sort of a bridge to have a CSI plugin? I've been checking if the go-plugin-helpers can be updates to act as a bridge between docker and a CSI enabled plugin but am not sure how much it helps if Docker itself can't take advantage of the CSI interfaces themselves.

akutz commented 6 years ago

Hi @govint,

We've already created a bridge to CSI for existing Docker Volume plug-ins. It's built into REX-Ray. I'm talking about creating a native CSI storage plug-in (SP) for vSphere. Specifically it's based on your Docker Volume VMDK driver.

This isn't for Docker, it's for all COs that will soon support CSI, such as K8s, Mesos, and yes, Docker too.

shuklanirdesh82 commented 6 years ago

upcoming release is already planned out for this Friday, this task will be taken care by upcoming release with highest priority.

Thanks @akutz once again!

govint commented 6 years ago

Will start this week on this one,

  1. First identify whats not used and remove those from the repo.
  2. Migrate to the new packages.