Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Imported metadata preservation #845

Closed
carolynvs opened this issue Jul 18, 2017 · 9 comments
Closed

Imported metadata preservation #845

carolynvs opened this issue Jul 18, 2017 · 9 comments

Comments

@carolynvs
Copy link
Collaborator

carolynvs commented Jul 18, 2017

NOTE: This issue was converted to an epic (#845 (comment)).


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
Copy link
Member

sdboyer commented Jul 18, 2017

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
Copy link
Collaborator Author

carolynvs commented Jul 18, 2017

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
Copy link

tmichel commented Jul 24, 2017

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
Copy link
Collaborator Author

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

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 25, 2017

@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
Copy link
Collaborator Author

#821 covers importing config on the fly during dep ensure.

@carolynvs
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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 carolynvs added the Epic label Oct 31, 2017
@carolynvs carolynvs changed the title Import should create overrides for transitive dependencies Clearly warn during init when imported metadata could not be preserved (EPIC) Oct 31, 2017
@carolynvs
Copy link
Collaborator Author

carolynvs commented Oct 31, 2017

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:

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

@carolynvs carolynvs added the Epic label Oct 31, 2017
@carolynvs carolynvs changed the title Clearly warn during init when imported metadata could not be preserved (EPIC) Imported metadata preservation Oct 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants