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 consul_node not detecting changes #104

Merged
merged 5 commits into from
May 26, 2019

Conversation

remilapeyre
Copy link
Collaborator

consul_node were not updating their address and meta attributes during plans. This let another bug go unnoticed where consul_service could change consul_node attributes when external would be set to true (see #101).

Multiple services could have conflicting configuration:

resource "consul_node" "compute" {
	name    = "compute-example"
	address = "www.hashicorptest.com"
}

resource "consul_service" "example1" {
	name = "example"
	node = "${consul_node.compute.name}"
	port = 80

	external = true
}


resource "consul_service" "example2" {
	name = "example"
	node = "${consul_node.compute.name}"
	port = 80

	external = true
}

// What should be the node's meta.external-probe and meta.external-node?

This patch fix resourceConsulNodeRead() and deprecate external from consul_service.
Another option would have been move external to the consul_node resource but it would then have conflicted with the meta attribute. We could use a custom DiffSuppressFunc to avoid this but I'm not sure it is worth the maintenance cost.

@Shaiou
Copy link

Shaiou commented May 6, 2019

Hi, any news as to when this will be merged ?

@remilapeyre
Copy link
Collaborator Author

Hi @Shaiou, thanks for the ping ! We should be able to cut a new release next week :)

@Shaiou
Copy link

Shaiou commented May 20, 2019

Hi, any news regarding the merge ?

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.

Nice, looks good. We have an upgrade guide -- should we document this change there? I assume anyone using external = true will have to do something differently now.

@remilapeyre remilapeyre merged commit 19f470f into hashicorp:master May 26, 2019
@Shaiou
Copy link

Shaiou commented May 28, 2019

👍

@Shaiou
Copy link

Shaiou commented May 28, 2019

Sorry to bother again, is there going to be a release/tag soon ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants