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

Fix #4076 - Agent configured Watches now work with HTTPS only agents #4184

Merged
merged 3 commits into from
Jun 1, 2018

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented May 31, 2018

Basically just needed to push the TLS setup into the Watch Plan Run method so when it generates the API Client it tries to use TLS.

Fixes #4076

mkeeler added 2 commits May 31, 2018 17:07
…API client

This allows watches from consul agent config (rather than consul watch command) to be able to utilize HTTPs
Copy link
Contributor

@kyhavlov kyhavlov 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, just one question 👍

watch/plan.go Outdated
@@ -20,10 +20,12 @@ const (
)

// Run is used to run a watch plan
func (p *Plan) Run(address string) error {
func (p *Plan) Run(address string, conf *consulapi.Config) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth keeping this signature the same and moving to a RunWithConfig method so we don't break tools that import this package. It is pretty small, though - what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats probably best. Nothing in our code does but I guess the api can be imported by other tools.

@mkeeler mkeeler merged commit 41f4bb4 into master Jun 1, 2018
@mkeeler mkeeler deleted the bugfix/gh-4076 branch June 1, 2018 14: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.

2 participants