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

Fix -no-vendor flag for ensure -update as well #1361

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

asticode
Copy link

@asticode asticode commented Nov 9, 2017

Currently the -no-vendor flag doesn't work on dep ensure -update.

This PR fixes it.

@asticode
Copy link
Author

The tests seem to be failing for other reasons than this commit. If you want me to add some test for the function I've added, just let me know :)

@carolynvs
Copy link
Collaborator

Sorry for the confusion, we have been having stability problems with our CI tests lately. The AppVeyor build failure is not from your PR and I don't think the Travis one is either. I've kicked travis to see if I can get it pretty and green but don't worry I don't believe there's anything you need to change on your end!

@darkowlzz
Copy link
Collaborator

@asticode Thanks for the PR. Can you add a test for this? Like the ones in https://github.com/golang/dep/tree/master/cmd/dep/testdata/harness_tests/ensure/update

@asticode
Copy link
Author

@darkowlzz all done. Is that what you had in mind?

Copy link
Collaborator

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Test looks good 👍

name = "github.com/sdboyer/deptest"
packages = ["."]
revision = "dummy"
version = "dummy"
Copy link
Collaborator

@darkowlzz darkowlzz Nov 15, 2017

Choose a reason for hiding this comment

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

Let's set these to some proper values from https://github.com/sdboyer/deptest/releases.

@@ -0,0 +1,12 @@
// Copyright 2016 The Go Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

New file: 2017 😊

@asticode
Copy link
Author

@darkowlzz all done

@asticode asticode force-pushed the patch-1 branch 2 times, most recently from 755012f to c343662 Compare November 15, 2017 12:57
@darkowlzz
Copy link
Collaborator

@asticode nice. Also, we need this feature in the CHANGELOG. Sorry that I forgot to mention that before.

@asticode
Copy link
Author

@darkowlzz I've updated the changelog but the PR has now a minor conflict with the master/CHANGELOG.md. Do you want me to fix it or should you do it upon merging the PR?

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@ BUG FIXES:

* Releases targeting Windows now have a `.exe` suffix (#1291).
* Adaptively recover from dirty and corrupted git repositories in cache (#1279).
* Fix `-no-vendor` flag for `ensure -update` ([#1361](https://github.com/golang/dep/pull/1361))
Copy link
Collaborator

Choose a reason for hiding this comment

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

A period (.) at the end and the full link is not required, just #1361 is enough.

@darkowlzz
Copy link
Collaborator

@asticode rebase this PR on master and resolve the conflict :)

@asticode
Copy link
Author

@darkowlzz all good now :)

Copy link
Collaborator

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

@sdboyer
Copy link
Member

sdboyer commented Nov 17, 2017

quick rebase and we can merge

Copy link
Collaborator

@ibrasho ibrasho left a comment

Choose a reason for hiding this comment

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

LGTM

@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@ibrasho
Copy link
Collaborator

ibrasho commented Nov 17, 2017

I tried to resolve the conflict through the web editor but that didn't work. 😕 You should remove my commit since it won't be useful when rebasing.

Just rebase from latest master to allow merging.

@asticode
Copy link
Author

@ibrasho done

@darkowlzz
Copy link
Collaborator

Closing and reopening to trigger the cla bot.

@darkowlzz darkowlzz closed this Nov 17, 2017
@darkowlzz darkowlzz reopened this Nov 17, 2017
@asticode
Copy link
Author

seems like the cla status will not change:

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@darkowlzz
Copy link
Collaborator

We need some super power here @sdboyer .

@sdboyer sdboyer merged commit 3670e70 into golang:master Nov 17, 2017
@sdboyer
Copy link
Member

sdboyer commented Nov 17, 2017

done, thank you!

@asticode asticode deleted the patch-1 branch November 17, 2017 14:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants