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

Implement new spec for `dep ensure` #489

Merged
merged 39 commits into from Aug 4, 2017

Conversation

@sdboyer
Copy link
Member

sdboyer commented Apr 30, 2017

This PR is a WIP towards implementing the new spec for dep ensure, as given in the doc linked from #277.

Fixes #883

sdboyer added some commits Apr 21, 2017

Further flesh out -add path
Still missing varied error handling on the other side of the solve; need
a bitfield to represent all of the state properties we infer about each
spec argument.

@sdboyer sdboyer force-pushed the sdboyer:new-ensure branch from 1b8edb3 to 6f783e0 May 9, 2017

sdboyer added some commits May 10, 2017

@sectioneight
Copy link
Contributor

sectioneight left a comment

a bit over my head for now, just one small thing popped out

applyUpdateArgs(args, &params)
} else {
err := applyEnsureArgs(ctx.Loggers.Err, args, cmd.overrides, p, sm, &params)
var fail error

This comment has been minimized.

@sectioneight

sectioneight May 16, 2017

Contributor

unused variable?

sdboyer added some commits Jun 5, 2017

@sdboyer sdboyer force-pushed the sdboyer:new-ensure branch from f8db1b0 to 824716b Jun 5, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jun 5, 2017

Pretty close to all tests passing, now - just three small harness ones left.

After that, docs, covering some more crucial failure cases, and writing up a ton of issues. Then this'll be ready to 🚢

@sdboyer sdboyer self-assigned this Jun 6, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 31, 2017

OK, we've got the basics in place! i also refactored to correctly validate and handle cases where multiple packages from a single project are specified.

now, i need to catch up and resolve conflicts, and then add a bunch more test cases. (and fix whatever bugs those tests reveal)

sdboyer added some commits Jul 31, 2017

@sdboyer sdboyer force-pushed the sdboyer:new-ensure branch from e405d48 to 97441d7 Jul 31, 2017

func (cmd *ensureCommand) Args() string { return "[spec...]" }
func (cmd *ensureCommand) Name() string { return "ensure" }
func (cmd *ensureCommand) Args() string {
return "[-update | -add] [-no-vendor | -vendor-only] [<spec>...]"

This comment has been minimized.

@ibrasho

ibrasho Jul 31, 2017

Collaborator

Shouldn't we add -examples and -dry-run here?

This comment has been minimized.

@sdboyer

sdboyer Jul 31, 2017

Member

oh yes, i missed -dry-run, tyty.

-examples...i'm not actually sure, it's a flavor of a help command. seems like those are often omitted in arg lists like these?

sdboyer added some commits Jul 31, 2017

Add a handful more harness tests
Also some comment cleanup, and relocate an errant test.

@sdboyer sdboyer requested a review from darkowlzz as a code owner Aug 1, 2017

@@ -261,7 +261,8 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana
var digestMismatch, hasMissingPkgs bool

if p.Lock == nil {
return digestMismatch, hasMissingPkgs, errors.Errorf("no Gopkg.lock found. Run `dep ensure` to generate lock file")
// TODO if we have no lock file, do...other stuff
return digestMismatch, hasMissingPkgs, nil

This comment has been minimized.

@darkowlzz

darkowlzz Aug 3, 2017

Collaborator

Do not forget to rebase this 🙂

This comment has been minimized.

@sdboyer

sdboyer Aug 3, 2017

Member

ahh ty that must have happened in a git checkout --strategy=ours

This comment has been minimized.

@sdboyer

sdboyer Aug 3, 2017

Member

this also indicates a detailed review of the changes to existing files is necessary - i may have unintentionally overwritten some other changes, too.

This comment has been minimized.

@sdboyer

sdboyer Aug 3, 2017

Member

should be fixed, i redid the merge without --strategy=ours, that was dumb

@sdboyer sdboyer force-pushed the sdboyer:new-ensure branch from 38214fd to 7d21ee7 Aug 3, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 4, 2017

here we go!

@sdboyer sdboyer changed the title [WIP] Implement new spec for `dep ensure` Implement new spec for `dep ensure` Aug 4, 2017

@sdboyer sdboyer merged commit 7a91b79 into golang:master Aug 4, 2017

6 of 7 checks passed

codeclimate 19 issues to fix
Details
cla/google All necessary CLAs are signed
codeclimate/coverage 74.77% (-0.3%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

sdboyer added a commit to sdboyer/dep that referenced this pull request Aug 11, 2017

carolynvs added a commit that referenced this pull request Aug 13, 2017

Merge pull request #991 from sdboyer/update-readme
Bring README up to date after #489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment