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

move to dep #1009

Merged
merged 4 commits into from
Nov 13, 2017
Merged

move to dep #1009

merged 4 commits into from
Nov 13, 2017

Conversation

adnxn
Copy link
Contributor

@adnxn adnxn commented Oct 7, 2017

Summary

Use dep instead of godep.

$ dep version
dep:
version : v0.3.2
build date : 2017-10-19
git hash : 8ddfc8a
go version : go1.9
go compiler : gc
platform : linux/amd64

Implementation details

See agent/Gopkg.toml and agent/Gopkg.lock.

Testing

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:
no

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License:
yes

@adnxn adnxn requested review from samuelkarp and petderek October 8, 2017 16:33
@petderek petderek mentioned this pull request Oct 9, 2017
8 tasks
agent/Gopkg.toml Outdated

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

Choose a reason for hiding this comment

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

What do you think about locking everything to the commit that is referenced in the old Godeps.json file? It would potentially reduce the blast radius.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about locking dependencies to tags and upgrade to nearest tagged version where it makes sense. ill add a separate commit for each dependency i lock to a new version. cause as we were saying earlier, tagging to revisions doesn't really provide guarantees. we can definitely do some things to reduce blast radius though.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming that the Gopkg.lock contains the same commits as the previous Gopkg.json file had for our deps I think it's okay to migrate this way.

Copy link
Contributor

@petderek petderek left a comment

Choose a reason for hiding this comment

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

I'm fine with either strategy we discussed.

@vsiddharth
Copy link
Contributor

Quoting the original document from dep

dep is a prototype dependency management tool for Go. It requires Go 1.8 or newer to compile.
dep is the official experiment, but not yet the official tool. 

Even though dep is safe for production, I'm not totally sure of switching to it yet!

@adnxn
Copy link
Contributor Author

adnxn commented Oct 11, 2017

It requires Go 1.8 or newer to compile.

Even though dep is safe for production, I'm not totally sure of switching to it yet!

hrm that's true, we should consider this.

/cc @aaithal, @jahkeup

what do you think?

@jahkeup
Copy link
Member

jahkeup commented Oct 11, 2017

The Makefile will also need to be updated to account for this change, which will require that we also bump the version of go used for the travis build. Though I'm happy to start using dep, we need to verify we are ready to update go. This looks great otherwise 👍

@samuelkarp
Copy link
Contributor

I don't think we need to bump Go. I think the dep tool needs Go 1.8 to be compiled itself, but our code should only rely on Go >= 1.5 (with GO15VENDOREXPERIMENT).

@samuelkarp
Copy link
Contributor

This now needs a rebase, since Godeps.json has been modified.

@adnxn adnxn added this to the 1.15.3 milestone Nov 8, 2017
this commit also removes Godep directory and old vendor directory
this commit adds revision constraints to keep parity with the old godep
vendor package versions and ran dep ensure
ran dep prune to remove unused packages from the vendor tree.
safe to remove, our automated process do not depend on godep executable
Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

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

Changes LGTM 🚀

  • Document the new behavior someplace
  • Use versions for constraints when applicable (outside this PR)

agent/prune.out Outdated
@@ -0,0 +1,6094 @@
On branch move-to-dep
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this file from this intermediate commit?

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Looks good after you remove that file from the intermediate commit. Thanks for doing this!

@samuelkarp samuelkarp mentioned this pull request Nov 12, 2017
18 tasks
Copy link
Contributor

@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.

:shipit:

@adnxn adnxn merged commit dab07f6 into aws:dev Nov 13, 2017
adnxn added a commit that referenced this pull request Nov 13, 2017
@nmeyerhans nmeyerhans modified the milestones: 1.15.3, 1.16.0 Nov 15, 2017
@adnxn adnxn deleted the move-to-dep branch March 14, 2018 17:46
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.

7 participants