Dep: Imported metadata preservation

[carolynvs]

NOTE: This issue was converted to an epic (https://github.com/golang/dep/issues/845#issuecomment-340791533).

---

Currently when importing from external tools, the rootAnalyzer deletes imported constraints for transitive dependencies. The original intent was to not import unused dependencies or create ineffective constraints (since constraints don't apply to transitive deps). We shouldn't be throwing away the constraint, and instead of unconditionally removing the constraint, we should check if the package is used, and if it is used, convert it to an override instead.

Since there are many importers and this logic (check if it's used, if it's a direct/transitive dep) is repetitive, this seems like a good candidate for continuing to live in the rootAnalyzer, keeping the importers straightforward.

[sdboyer]

If possible, I really want to avoid creating overrides by default. Given that we want to discourage their use in general, I think it may send the wrong message to users to be doing it automatically.

I'd rather we try for doing tool conversions on the fly, as entailed by #821. If that works well, it should achieve largely equivalent results, without necessitating overrides.

[carolynvs]

Adding @chriswhelix as he is who brought this to my attention on community day. 😁

While I understand not wanting to encourage people to use overrides, I really do not want to ignore valid external configuration during import. If a user was using glide/govendor/whatever to explicitly set a constraint on a transitive dependency, it doesn't seem right to assume that they didn't need it and throw it away. If we say that we support a tool, during import we should replicate the effect of that tool's config to the best of our ability.

Ignoring #821 for a moment, I think this is a problem with dep init.

1. I tested github.com/sdboyer/deptest and found out that there's a problem with v1.0.0 in my app, so I set the version in the glide.yaml to v0.8.1.
2. When I run glide up, it doesn't automatically bump me to a version that I know doesn't work. Huzzah.
3. Now I run dep init. The correct revision for deptest is set in the Gopkg.lock initially so I think the conversion worked fine.
4. The next time I run dep ensure -update, since there isn't an override in the manifest, deptest is updated to the known bad version v1.0.0, and I'm left wondering why.

Here's what dep init does now and what I am suggesting that it should do:

glide.yaml

import:
- package: github.com/sdboyer/deptestdos
  version: master
- package: github.com/sdboyer/deptest
  version: v0.8.1

glide.lock

imports:
- name: github.com/sdboyer/deptestdos
  version: a0196baa11ea047dd65037287451d36b861b00ea
- name: github.com/sdboyer/deptest
  version: 3f4c3bea144e112a69bbe5d8d01c1b09a544253f

Current Gopkg.toml

[[constraint]]
  branch = "master"
  name = "github.com/sdboyer/deptestdos"

Desired Gopkg.toml

[[constraint]]
  branch = "master"
  name = "github.com/sdboyer/deptestdos"

[[override]]
  name = "github.com/sdboyer/deptest"
  version = "0.8.1"

[tmichel]

Automatically pinning transitive dependencies when importing would really come handy for me. I'm using govendor at the moment and tried to switch to dep but it breaks the application because transitive dependencies are not pinned but upgraded to their latest versions.

Here's the branch where I tried to upgrade: https://github.com/sspinc/terraform-provider-credstash/tree/dep
Unfortunately it only breaks at runtime. Let me know if you need more details.

[carolynvs]

@sdboyer Can you let me know if my clarification makes sense, and if you are okay with it?

[ibrasho]

@tmichel We rely on the dependencies to declare their constraints (and you use overrides to override these constraints). But, at the moment, we assume the dependencies use dep and only look for Gopkg.toml and Gopkg.lock. I remember seeing an issue suggesting that we utilize the importers on transitive dependencies.

[carolynvs]

https://github.com/golang/dep/issues/821 covers importing config on the fly during dep ensure.

[carolynvs]

One more aspect of this just came up: alternate sources for transitive dependencies. Currently, when we throw away imported transitive dependencies, we don't check if it was doing something important, such as specifying an alternate source.

[carolynvs]

Here’s a blog post that just highlights to me that this is needed. People shouldn’t have things work with godep and then it immediately breaks with dep because we ignored transitive deps.

https://medium.com/@andy.goldstein/upgrading-kubernetes-client-go-from-v4-to-v5-bbd5025fe381

[carolynvs]

After learning more about how overrides are treated during solve, I see now why we don't want to automatically make them during import. I think a few related issues may help with the "it used to work with glide/godep/etc but doesn't work with dep" problem:

• Warn when the imported metadata, e.g. a revision used by a transitive dependency, can't be honored. #907 covers constraints and #908 locked revisions.
• Automatically create a source-only override when an imported transitive dependency specifies an alternate source. #1333
• Document what importers keep always, try to keep and always ignore, and link to it from any errors.

I've converted this to an epic and will use this to track the various issues.