New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support importing glide config during dep init #380

Closed
carolynvs opened this Issue Apr 14, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@carolynvs
Copy link
Collaborator

carolynvs commented Apr 14, 2017

  • dep init checks for existing Glide configuration files (glide.yaml/glide.lock) and uses them to populate the manifest and lock.
  • Specify -skip-tools to skip importing existing dependency management config from other tools.

This is part of the epic #186.

Behaviors

  • We only look for glide.yaml at the root of the project.
  • If a project is discovered by dep init that was not configured in glide.yaml, it is still included in the manifest. Note: Fixing #149 is out of scope for this issue.
  • When version/branch is specified in glide.yaml, use that value in the manifest.
  • When repo is specified in glide.yaml, use that value as the source in the manifest.
  • When a list of subpackages are specified in the glide.yaml, ignore them since dep will generate that list using static analysis.
  • When an ignore list is specified in glide.yaml, use those values in the manifest.
  • Packages in testImports are included in the manifest as regular [[dependencies]], i.e. no distinction is made between imports and test imports.
  • When excludeDirs, import.os or import.arch is specified in glide.yaml, warn that it isn't supported.
  • Ignore homepage, license or owners is specified in glide.yaml.
  • Lock to the same revision that is specified in glide.lock.
  • When the revisions from the glide configuration do not meet the constraints, what should we do?
    1. Prompt the user to either create an override or use some revision suggested by dep?
    2. Create an override?
    3. Is there another option?
  • When configuration files from multiple dependency managers is found (e.g. glide AND godep), what is the expected behavior. Not a problem for this PR, but it is helpful to know ahead of time.
@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 14, 2017

I've updated the issue with my first stab at how this command should behave but have a few questions though that will need clarification.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 14, 2017

AAAAAAAAAAWESOME QUESTIONS 🎉 🎉

  • We only look for glide.yaml at the root of the project.
  • When repo is specified in glide.yaml, use that value as the source in the manifest.
  • When a list of subpackages are specified in the glide.yaml, ignore them since dep will generate that list using static analysis.
  • When an ignore list is specified in glide.yaml, use those values in the manifest.
  • Packages in testImports are included in the manifest as regular [[dependencies]], i.e. no distinction is made between imports and test imports.
  • Ignore homepage, license or owners is specified in glide.yaml.

👍

  • If a project is discovered by dep init that was not configured in glide.yaml, it is still included in the manifest.

Yes, but also, the inverse case - constraints given in glide.yaml without a corresponding imports should not make it into the manifest.

Note: Fixing #149 is out of scope for this issue.

If you can do this without touching #149 yet, then great 😄

  • When version/branch is specified in glide.yaml, use that value in the manifest.

So, this is actually a problem point. glide doesn't differentiate between constraint types in its manifest, which makes input untyped and ambiguous. Case in point - Masterminds/semver has a branch named 2.x, which is...also a valid semver wildcard pattern.

There was a kerfuffle over this: Masterminds/glide#391

This is actually the same problem we currently have with constraints passed to ensure on the command line. glide has, historically, resolved this roughly the same way do - by reaching out to the target repository and comparing the input against a list of versions. We need to do the same here.

  • Lock to the same revision that is specified in glide.lock.

A bit more work to do here, as well - we need to reach out and try to pair that revision with a tag or branch, if possible.

  • When excludeDirs, import.os or import.arch is specified in glide.yaml, warn that it isn't supported.

excludeDirs should be able to be converted to ignores, I think.

  • When the revisions from the glide configuration do not meet the constraints, what should we do?
    1. Prompt the user to either create an override or use some revision suggested by dep?
    2. Create an override?
    3. Is there another option?

Depends on which part of the glide configuration. If it's the versions from the lock, that shouldn't be a problem - the solver will sort that out.

If it's the versions from the manifest, though, that's trickier, and fairly involved. However, it's VERY unlikely to a be a situation encounters anytime soon, so I'm fine with making a specific issue for it and kicking it down the road.

  • When configuration files from multiple dependency managers is found (e.g. glide AND godep), what is the expected behavior. Not a problem for this PR, but it is helpful to know ahead of time.

Blow up 💣 💥 😅

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 17, 2017

constraints given in glide.yaml without a corresponding imports should not make it into the manifest.

Is there any scenario where Required is populated from the glide config? Perhaps when they are using a dev tool that's specified in the glide.yaml, like golint, but isn't imported in any of their project's code?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 18, 2017

I don't think so, as I believe glide's still relying on static analysis to determine what's actually needed (same as gps).

I'd say it'd be fine to print a message for each ineffectual constraint the conversion encounters, though.

@kevingo

This comment has been minimized.

Copy link

kevingo commented Apr 18, 2017

I just curiosity about why only support importing from glide instead of other existing tools to dep?

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 18, 2017

@kevingo This issue is just for glide, see #186 for the epic. We plan on supporting more, glide is just the first one to be worked on.

@kevingo

This comment has been minimized.

Copy link

kevingo commented Apr 18, 2017

@carolynvs Thanks for reply.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 18, 2017

Folks are welcome to jump in on the implementations for others at any time 😄 As I mention in #186, there's mostly-working implementations for several existing pkg mgrs out there already (I wrote them when converting glide to gps last fall)

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 21, 2017

@sdboyer Do you have a preference on whether or not the importers are defined in sub-package, e.g. github.com/golang/dep/importer? I started down that path initially then realized that I wanted things (like dep.vlogf). Depending on whether or not these importers are all in a sub-package will really change how I go about doing that! 😀

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 22, 2017

@carolynvs i don't really have a strong preference. i do think they should be unexported, at least for now.

In general, I think we need to start leaning more towards not exporting things we don't have to - or putting them in an internal dir - as we move more towards stability.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Jun 19, 2017

Fixed by #500

@carolynvs carolynvs closed this Jun 19, 2017

@marcellodesales

This comment has been minimized.

Copy link

marcellodesales commented Feb 20, 2018

So I can't init... @carolynvs

  • Glide File
  [root@1db06bcd2334]:intuit-k8s-cli # cat glide.yaml
package: github.com/intuit/intuit-k8s-cli
import:
- package: github.com/spf13/viper
- package: github.com/Sirupsen/logrus
  • Tried to migrate
  [root@1db06bcd2334]:intuit-k8s-cli # dep init
Importing configuration from glide. These are only initial constraints, and are further refined during the solve process.
Detected glide configuration files...
Converting from glide.yaml...
Username for 'https://github.com': marcellodesales

^CSignal received: waiting for 1 ops to complete...
^C
  • Why does it need to ask for my github username to download public images? Am I missing anything?
@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Feb 20, 2018

@marcellodesales Would you please open a new issue? That will make it easier to track down what's going on.

@golang golang locked as resolved and limited conversation to collaborators Feb 20, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.