Skip to content
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

Migrate to Dep #317

Merged
merged 5 commits into from
Feb 13, 2018
Merged

Migrate to Dep #317

merged 5 commits into from
Feb 13, 2018

Conversation

SoManyHs
Copy link
Contributor

@SoManyHs SoManyHs commented Sep 8, 2017

make build succeeds

@samuelkarp
Copy link
Contributor

It looks like you now have vendor and ecs-cli/vendor. I think you should keep the dependencies under ecs-cli/vendor instead of in the root.

@samuelkarp
Copy link
Contributor

  • github.com/hashicorp/go-cleanhttp seems to have disappeared. Do we no longer need it?
  • You have both github.com/Sirupsen/logrus and github.com/sirupsen/logrus. This is going to be bad for anyone with a case-insensitive filesystem. I think they also technically count as different imports in Go, which isn't great either.
  • golang.org/x/crypto looks like a new dependency.

@SoManyHs
Copy link
Contributor Author

@samuelkarp, hmm, the logrus thing might be related to docker/libcompose#493.

@rifelpet
Copy link
Contributor

Yeah, we have a project of our own that depends on docker/libcompose as well as docker/docker and docker/cli, and we're blocked on moving to dep until they update their dependencies.

Although with this merged it may help but I havent tried yet. You may want to try updating your dep to the latest master and trying the migration again.

@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from 0d31395 to ec85c18 Compare October 17, 2017 00:10
@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from d4e25ab to b7fe70d Compare October 30, 2017 19:53
@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from 145f9c2 to d7adfeb Compare November 9, 2017 23:17
@SoManyHs
Copy link
Contributor Author

@rifelpet Out of curiosity, have you tried migrating to dep again recently? The case-sensitivity issue with logrus appears to have been fixed -- I'm in the process of trying this migration process again and it appears to be working with the latest version of dep, though it required a lot of manual dependency resolution on my end.

@rifelpet
Copy link
Contributor

@SoManyHs I did see it was fixed but have not had the chance to try migrating again recently

@SoManyHs
Copy link
Contributor Author

Rebased on top of #412.

@SoManyHs SoManyHs requested a review from adnxn January 16, 2018 20:01
@aaithal
Copy link

aaithal commented Jan 16, 2018

what's left here for making this go from [wip] to not be wip anymore?

@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from f90f3c5 to 814156d Compare January 22, 2018 22:51
@aaithal
Copy link

aaithal commented Jan 22, 2018

This seems like a spurious folder: https://github.com/SoManyHs/amazon-ecs-cli/tree/dep/ecs-cli/_vendor-20180112100655. Was that added by mistake? Or, is that required? If required, why is it required?

@SoManyHs
Copy link
Contributor Author

@aaithal Oops, I thought I'd added that to the .gitignore but must have messed up. Should be fixed now--thanks for the catch!

@aaithal
Copy link

aaithal commented Jan 22, 2018

Can you also confirm if you need any changes to the scripts/license.sh file (because the vendor dir might have around a bit)?

@SoManyHs
Copy link
Contributor Author

@aaithal Yep, already on it.

@SoManyHs SoManyHs force-pushed the dep branch 3 times, most recently from 0491dae to 8d2f800 Compare January 29, 2018 23:20
Copy link

@adnxn adnxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SoManyHs i think we should explicitly constrain the dependencies to specific branch, version or revision in the Gopkg.toml file.

Updated major dependencies (libcompose, golang.org/x/*,
docker, Azure/go-ansiterm), largely to get rid of case-sensitive
dependency collision of Sirupsen.
@SoManyHs SoManyHs changed the title [WIP] Migrate to Dep Migrate to Dep Feb 6, 2018
@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from fd7c7cc to 97dcef4 Compare February 7, 2018 00:38
@@ -0,0 +1,66 @@

# Gopkg.toml example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this example code be removed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the examples here are useful since there are a few ways to constrain your dependencies.



[[constraint]]
name = "github.com/aws/aws-sdk-go"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per @adnxn's comment, can you lock all of the dependencies on a specific version or githash, etc?

- From ecs-cli dir, ran `dep init`
- Removed Godeps dir
- Ran `make generate` (to update licenses)
Ran `dep ensure && dep prune` to get rid of unused vendor files added by
`dep init` (dep v0.3.2)
- Updated Gopkg.toml manually (NOTE: Using the "revision" constraint is
  not usually recommended, but it was the easiest to do for the moment
  and would ensure consistency with what was previously in the Godeps
  manifest)
- Ran dep ensure && dep prune (dep v0.3.2)
@SoManyHs
Copy link
Contributor Author

Okay, updated.

@SoManyHs SoManyHs force-pushed the dep branch 2 times, most recently from e40e223 to 402889b Compare February 13, 2018 07:07
Copy link

@aaithal aaithal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SoManyHs SoManyHs merged commit 402889b into aws:dev Feb 13, 2018
@SoManyHs SoManyHs deleted the dep branch February 13, 2018 21:27
@adnxn
Copy link

adnxn commented Feb 13, 2018

we can close this now #282

@SoManyHs
Copy link
Contributor Author

Good catch. I'll close it when we next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants