add cloud auto join provider: tencentcloud and update vendor #6818

likexian · 2019-11-20T02:33:21Z

update tests and docs.

banks

@likexian thanks for contributing this!

CI is failing because vendor is not updated correctly. Can you run make update-vendor to correctly process vendoring please?

I'm also wary of adding replace directives to go.mod - is that needed? If so I'd like to understand why and if we can get things compatible without it as it becomes hard to predict what pinning to that other fork and commit will do to other transient dependencies.

banks · 2019-11-20T12:40:41Z

go.mod

@@ -6,6 +6,8 @@ replace github.com/hashicorp/consul/api => ./api

 replace github.com/hashicorp/consul/sdk => ./sdk

+replace launchpad.net/gocheck => github.com/go-check/check v0.0.0-20190902080502-41f04d3bba15


@likexian is this necessary? I'm wary of adding replace directives to the whole of Consul without knowing exactly why they are needed.

on launchpad.net website, it said that gocheck has been moved to github, please check it via: https://launchpad.net/gocheck

however I have revert it to avoid the worried.

I see. I think it's better to leave it as it was though otherwise we could be pulling in a newer version that the dependency intended. If there is concern about the launchpad version going away then the dependency that imported it should probably have a PR to switch their import rather than us second guessing which one they want here.

Thanks for reverting!

likexian · 2019-11-21T02:18:01Z

@likexian thanks for contributing this!

CI is failing because vendor is not updated correctly. Can you run make update-vendor to correctly process vendoring please?

I am sure I have run make update-vendor before I commit, but it seems that the CI runs into different result. I check some of the diffs, it seems that just the issues of order, I don't know how to resolve, please help me, for example:

EDIT: well, it seems that it is the difference of go version, I used go1.13 but CI go1.12, I have fixed it.

banks · 2019-11-21T12:35:52Z

EDIT: well, it seems that it is the difference of go version, I used go1.13 but CI go1.12, I have fixed it.

Thanks for checking that out! I'll note this so the team knows. We hope to upgrade build system soon but 1.13 broke some of our build tools.

banks · 2019-11-21T12:38:14Z

.circleci/config.yml

@@ -31,6 +31,7 @@ jobs:
      - image: *GOLANG_IMAGE
    steps:
      - checkout
+      - run: sudo apt-get install -y bzr


@alvin-huang do we want to do this here? I think we should probably add this to the make tools instead right?

Also, I wonder if the replace directive I asked @likexian to remove which redirects off launchpad.net would also fix this. Which is worse having every contributor need bzr or maintaining a redirect in go mod?

When we get to 1.13 and use a module proxy this will also go away again...

I think maintaining the redirect is better than having others install bzr. I don't see that line changing much between now and when we get to 1.13 so it's minimal maintenance.

@banks @alvin-huang
So, what can I do for this PR to go on?

Apologies Li,

We've been waiting to get consensus on the best approach here before we ask you to change anything else again as it's not ideal to keep changing our minds!

US folks are out this week but I'll try to get back to you next week so we can move this on!

Thanks again for you contribution.

Thanks for the reply, If there is something I can do, it is my pleaseure.

Transitive dep, source at https://launchpad.net/gocheck indicates project moved. This also avoids a dependency on bzr when fetching modules. Refs #6818

banks

Hey @likexian just so you're clear, we decided collectively the replace was in fact the least bad option here going forward (apologies for the delay in figuring that out when you did it from the start), but we also had another update from go-discover to merge in.

Mike has done that in #6865 which includes the replace in go mod and vendor updates etc.

Would you be able to rebase this once he merges to master? Then we should be left with only the docs additions which would be perfect and can be merged right away!

Thanks!

Refs hashicorp/go-discover#128 * deps: add replace directive for gocheck Transitive dep, source at https://launchpad.net/gocheck indicates project moved. This also avoids a dependency on bzr when fetching modules. Refs #6818 * deps: make update-vendor * test: update retry-join expected names from go-discover

likexian · 2019-12-05T02:45:58Z

Hey @likexian just so you're clear, we decided collectively the replace was in fact the least bad option here going forward (apologies for the delay in figuring that out when you did it from the start), but we also had another update from go-discover to merge in.

Mike has done that in #6865 which includes the replace in go mod and vendor updates etc.

Would you be able to rebase this once he merges to master? Then we should be left with only the docs additions which would be perfect and can be merged right away!

Thanks!

Well, I have rebase it.

codecov · 2019-12-05T07:20:29Z

Codecov Report

Merging #6818 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6818      +/-   ##
==========================================
+ Coverage   65.83%   65.85%   +0.01%     
==========================================
  Files         435      435              
  Lines       52488    52488              
==========================================
+ Hits        34558    34566       +8     
+ Misses      13796    13785      -11     
- Partials     4134     4137       +3

Impacted Files	Coverage Δ
api/session.go	`66.07% <0%> (-5.36%)`	⬇️
agent/consul/acl_replication_legacy.go	`83.05% <0%> (-3.39%)`	⬇️
agent/cache/watch.go	`78.75% <0%> (-2.5%)`	⬇️
api/kv.go	`83.45% <0%> (-1.44%)`	⬇️
api/watch/funcs.go	`74.61% <0%> (-1.04%)`	⬇️
agent/local/state.go	`79.93% <0%> (-0.87%)`	⬇️
command/exec/exec.go	`66.56% <0%> (-0.6%)`	⬇️
agent/consul/rpc.go	`78.57% <0%> (-0.38%)`	⬇️
agent/consul/connect_ca_endpoint.go	`57.19% <0%> (ø)`	⬆️
agent/config/builder.go	`83.48% <0%> (+0.08%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6817676...0d056db. Read the comment docs.

banks

Thanks @likexian for all your help here!

This should be included in our 1.7.0 release shortly!

likexian · 2019-12-05T12:37:23Z

Thank you.

ghost · 2020-01-25T02:01:50Z

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

likexian mentioned this pull request Nov 20, 2019

Add support for Tencent Cloud to auto-join #6812

Closed

banks requested changes Nov 20, 2019

View reviewed changes

banks reviewed Nov 21, 2019

View reviewed changes

mikemorris added a commit that referenced this pull request Dec 2, 2019

deps: add replace directive for gocheck

b944a64

Transitive dep, source at https://launchpad.net/gocheck indicates project moved. This also avoids a dependency on bzr when fetching modules. Refs #6818

mikemorris mentioned this pull request Dec 2, 2019

Bump go-discover to support EC2 Metadata Service v2 #6865

Merged

banks requested changes Dec 4, 2019

View reviewed changes

add tencentcloud auto join docs

0d056db

banks approved these changes Dec 5, 2019

View reviewed changes

banks merged commit a8b3be0 into hashicorp:master Dec 5, 2019

ghost locked and limited conversation to collaborators Jan 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cloud auto join provider: tencentcloud and update vendor #6818

add cloud auto join provider: tencentcloud and update vendor #6818

likexian commented Nov 20, 2019

banks left a comment

banks Nov 20, 2019

likexian Nov 21, 2019

likexian Nov 21, 2019

banks Nov 21, 2019

likexian commented Nov 21, 2019 •

edited

Loading

banks commented Nov 21, 2019

banks Nov 21, 2019

alvin-huang Nov 21, 2019

likexian Nov 29, 2019

banks Nov 29, 2019

likexian Nov 30, 2019

banks left a comment

likexian commented Dec 5, 2019

codecov bot commented Dec 5, 2019

banks left a comment

likexian commented Dec 5, 2019

ghost commented Jan 25, 2020

		@@ -6,6 +6,8 @@ replace github.com/hashicorp/consul/api => ./api

		replace github.com/hashicorp/consul/sdk => ./sdk

		replace launchpad.net/gocheck => github.com/go-check/check v0.0.0-20190902080502-41f04d3bba15

add cloud auto join provider: tencentcloud and update vendor #6818

add cloud auto join provider: tencentcloud and update vendor #6818

Conversation

likexian commented Nov 20, 2019

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likexian commented Nov 21, 2019 • edited Loading

banks commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banks left a comment

Choose a reason for hiding this comment

likexian commented Dec 5, 2019

codecov bot commented Dec 5, 2019

Codecov Report

banks left a comment

Choose a reason for hiding this comment

likexian commented Dec 5, 2019

ghost commented Jan 25, 2020

likexian commented Nov 21, 2019 •

edited

Loading