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.

Sign up for GitHub

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

Jump to bottom

Vendoring update for go-discover. #4412

Merged
merged 8 commits into from
Jul 25, 2018
Merged

Vendoring update for go-discover. #4412

merged 8 commits into from
Jul 25, 2018

Conversation

MagnumOpus21
Copy link
Contributor

Changes made in this PR include:

  • Added two new providers : vSphere and packer.
  • Fixed misc. changes made to go-discover.

Fixes #4408 .

@MagnumOpus21 MagnumOpus21 requested a review from pearkes July 18, 2018 16:01
@banks
Copy link
Member

banks commented Jul 18, 2018

@MagnumOpus21 I assume the vendor update is legit although it's a ton of new code - I guess becuase new transitive deps on large vmware SDK or something.

Can you add the command you used to update this to the PR please? It's always worth sanity checking as a few PRs recently have pulled in way too much stuff with a slight vendor command slip up.

Also, are the new vendors added actually usable in Consul after this PR? If so, can we update the docs too please?

@MagnumOpus21
Copy link
Contributor Author 

govendor fetch github.com/hashicorp/go-discover/provider/packet
govendor fetch github.com/hashicorp/go-discover/provider/vsphere 

govendor update github.com/hashicorp/go-discover

The first two commands were to add in the vSphere and the packet providers. The one after the newline was for updating the go-discover package. I was looking at kardianos/govendor looking at how to use them.

@MagnumOpus21
Copy link
Contributor Author

MagnumOpus21 commented Jul 18, 2018 •
edited
Loading 

github.com/Sirupsen/
github.com/hashicorp/go-discover/provider/packet/
github.com/hashicorp/go-discover/provider/vsphere/
github.com/hashicorp/go-discover/terraform.tfstate
github.com/hashicorp/go-discover/terraform.tfstate.backup
github.com/packethost/
github.com/vmware/
golang.org/x/crypto/ssh/terminal/

This was the git status after the addition of the providers.

@MagnumOpus21
Copy link
Contributor Author

@banks Regarding the documentation, doesn't go-discover's README.md contain instructions on how to use the providers? If I'm not in the right path, what documentation where you alluding to sir.

@mkeeler
Copy link
Member

mkeeler commented Jul 18, 2018

@MagnumOpus21 I think he meant adding something with the other cloud auto join docs: https://www.consul.io/docs/agent/cloud-auto-join.html

@MagnumOpus21
Copy link
Contributor Author

I can add the docs for them. ^_^

@MagnumOpus21
Copy link
Contributor Author

Updated the auto-join documentation. 👍

@MagnumOpus21
Copy link
Contributor Author

This needs the latest golang.org/x/sys/unix or else make will fail to produce any binary.

@pearkes
Copy link
Contributor

pearkes commented Jul 19, 2018

It looks like some Terraform tfstate files snuck in here:

vendor/github.com/hashicorp/go-discover/terraform.tfstate

And I don't see those in the go-discover repo so guessing they're on your local filesystem.

@MagnumOpus21
Copy link
Contributor Author

Remember the go-discover tests from earlier. They snuck in here. Let me remove them. LUL.

@pearkes pearkes added this to the 1.2.2 milestone Jul 19, 2018
pearkes
pearkes reviewed Jul 25, 2018
View reviewed changes
vendor/vendor.json
@@ -240,8 +262,8 @@
{"path":"golang.org/x/net/proxy","checksumSHA1":"QEm/dePZ0lOnyOs+m22KjXfJ/IU=","revision":"02ac38e2528ff4adea90f184d71a3faa04b4b1b0","revisionTime":"2017-07-18T17:12:47Z"},
{"path":"golang.org/x/net/trace","checksumSHA1":"u/r66lwYfgg682u5hZG7/E7+VCY=","revision":"d866cfc389cec985d6fda2859936a575a55a3ab6","revisionTime":"2017-12-11T20:45:21Z"},
{"path":"golang.org/x/sys/cpu","checksumSHA1":"REkmyB368pIiip76LiqMLspgCRk=","revision":"ad87a3a340fa7f3bed189293fbfa7a9b7e021ae1","revisionTime":"2018-06-18T16:37:21Z"},
{"path":"golang.org/x/sys/unix","checksumSHA1":"vlicYp+fe4ECQ+5QqpAk36VRA3s=","revision":"cd2c276457edda6df7fb04895d3fd6a6add42926","revisionTime":"2017-07-17T10:05:24Z"},
{"path":"golang.org/x/sys/windows","checksumSHA1":"Pki0iA65Dtsjh4gtabPkp/lOa2I=","revision":"cd2c276457edda6df7fb04895d3fd6a6add42926","revisionTime":"2017-07-17T10:05:24Z"},
{"path":"golang.org/x/sys/unix","checksumSHA1":"su2QDjUzrUO0JnOH9m0cNg0QqsM=","revision":"ac767d655b305d4e9612f5f6e33120b9176c4ad4","revisionTime":"2018-07-08T03:57:06Z"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any concern upgrading this dependency? If I understand correctly it has strict backwards compatibility promises but wanted to raise it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting the origin here: https://github.com/hashicorp/go-discover/blob/master/provider/vsphere/vendor/vendor.json#L26-L27

pearkes
pearkes approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@pearkes pearkes 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 to me. My only concern was with the change for the golang.org/x/sys packages the vsphere provider introduces. After talking to some folks, the promise should be that with the current version of Go compatibility should be retained. More here.

@pearkes pearkes merged commit f4a1c38 into hashicorp:master Jul 25, 2018
@bcmills bcmills mentioned this pull request Sep 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pearkes pearkes pearkes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@MagnumOpus21 @banks @mkeeler @pearkes