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

Fix associate_public_ip_address attr for EC2 #287

Merged
merged 1 commit into from
Dec 11, 2016

Conversation

diwaniuk
Copy link
Contributor

Fix hardcoded value for associate_public_ip_address attribute from "true" to dynamic based on public_ip_address state.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f638e6e on diwaniuk:ec2_associate_public_ip into d78824c on dtan4:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling f638e6e on diwaniuk:ec2_associate_public_ip into d78824c on dtan4:master.

@@ -96,6 +96,10 @@ def in_vpc?(instance)
(instance.subnet_id && instance.subnet_id != "" && instance.security_groups.empty?)
end

def associate_public_ip?(instance)
(instance.public_ip_address.to_s.empty?) ? "false" : "true"
end
Copy link
Owner

Choose a reason for hiding this comment

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

This method should return boolean value:

def associate_public_ip_address?(instance)
  !instance.public_ip_address.to_s.empty?
end

and both tf and tfstate should call this method.

tf:

associate_public_ip_address = <%= associate_public_ip_address?(instance) %>

tfstate:

"associate_public_ip_address?(instance).to_s

@diwaniuk
Copy link
Contributor Author

diwaniuk commented Dec 3, 2016

Fixed.

@@ -96,6 +96,10 @@ def in_vpc?(instance)
(instance.subnet_id && instance.subnet_id != "" && instance.security_groups.empty?)
end

def associate_public_ip?(instance)
(instance.public_ip_address.to_s.empty?) ? false : true
Copy link
Owner

Choose a reason for hiding this comment

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

hmm... String#empty? returns boolean value, true or false, so this can be written more simply,

!instance.public_ip_address.to_s.empty?

@dtan4
Copy link
Owner

dtan4 commented Dec 11, 2016

LGTM 👍 Thanks ❗️

@dtan4 dtan4 merged commit 13b7f33 into dtan4:master Dec 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants