Logrus import case sensitive patch (#8263)

[pokstad]

The spelling of "Sirupsen" is changed to "sirupsen" to avoid dependency management issues with vgo when vic is imported into other projects that use newer version of logrus.

See related issue: golang/go#26208

[vmwclabot]

@pokstad, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

[zjs]

Thanks for the pull request! Compatibility with vgo certainly seems like a good goal to work towards.

I would expect that we have other minor compatibility issues with newer versions of Go which may also need to be fixed — have you encountered any? It would be good to verify that we have appropriately-prioritized issues tracking those.

rename sirupsen folder to lowercase

With this commit (805f623), the version of logrus in our vendor directory is no longer the same as any upstream version of logrus. This may be enough to get things working (although it seems like any import statements in the files in the renamed directory would now have the wrong import path?), but seems incomplete.

I think it'd be even better to upgrade the vendored version of logrus to a more recent version, with the lower-case sirupsen. Upgrading dependencies is always a bit of a crapshoot though, and I'm not sure how invasive a change like that might become. Would you be interested in giving this a try?

[pokstad]

Thank you for taking time to review my PR.

I have not encountered any other issues. My primary exposure to this project has been through the VMware Terraform provider, and in that regard this has been the only issue that has popped up.

Re the renaming: I don't think the vendored logrus package source files will have incorrect imports, since I have also updated those import statements. In other words, everything under vendor/github.com/sirupsen/logrus/ should have the import paths rewritten to the new lowercase spelling. See this example from the PR: https://github.com/vmware/vic/pull/8264/files#diff-a7b587f95f2690b27e2d970a4084d9f7L7

Re updating the vendored dep: That does seem like an invasive change and I was trying to avoid that. I would be up for giving it a try but I'm still trying to wrap my mind around how your CI testing is done. Without a solid understanding of how to validate breaking changes, I would prefer to avoid this. Ideally, I think it is best to avoid updating a dependency unless there is something needed from the newer version aside from a superficial change like the import path spelling.

That being said, I could submit another PR with the updated logrus dep, but I don't know how dependency management is being done in this project. I didn't see any evidence of package management files (e.g. glide, dep, vgo) so I assume it is done in a manual/custom fashion.

[zjs]

I have not encountered any other issues. My primary exposure to this project has been through the VMware Terraform provider, and in that regard this has been the only issue that has popped up.

This is helpful context, thanks!

Re the renaming: I don't think the vendored logrus package source files will have incorrect imports, since I have also updated those import statements. In other words, everything under vendor/github.com/sirupsen/logrus/ should have the import paths rewritten to the new lowercase spelling. See this example from the PR: https://github.com/vmware/vic/pull/8264/files#diff-a7b587f95f2690b27e2d970a4084d9f7L7

I see that now. I think that should address my concern about imports from within the vendored package!

Re updating the vendored dep: That does seem like an invasive change and I was trying to avoid that. I would be up for giving it a try but I'm still trying to wrap my mind around how your CI testing is done. Without a solid understanding of how to validate breaking changes, I would prefer to avoid this.

There's some basic information in CONTRIBUTING.md. Many issues can be found using make check (which runs static analysis), make test (which runs unit tests), and make most (which compiles).

Our CI system runs these steps, plus a variety of integration tests. Out of an abundance of caution (e.g., to avoid denial of service-style attacks), our CI system won't trigger automatically for PRs by external contributors. We don't currently have a way to whitelist contributors, but any of us would be happy to manually trigger CI for your PR though (e.g., I ran this job a few hours ago — it looks like goimports is unhappy with the ordering of some lines following the case change).

Ideally, I think it is best to avoid updating a dependency unless there is something needed from the newer version aside from a superficial change like the import path spelling.
That being said, I could submit another PR with the updated logrus dep, but I don't know how dependency management is being done in this project. I didn't see any evidence of package management files (e.g. glide, dep, vgo) so I assume it is done in a manual/custom fashion.

We use gvt for dependency management. The commit messages for d7d9e24 and 0121c62 give some examples for how to use it if you're unfamiliar. (docs)

If you'd like to give this a try and see if updating the vendored library version works without much fuss, that would be awesome. If you'd rather just wrap this up as-is and have someone else investigate upgrading to a newer version, that's fine too!