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

Fixes for route53_records #303

Merged
merged 5 commits into from
Jan 20, 2017
Merged

Fixes for route53_records #303

merged 5 commits into from
Jan 20, 2017

Conversation

mioi
Copy link

@mioi mioi commented Jan 12, 2017 •
edited
Loading

This PR is for a couple of changes for resources of aws_route53_record type:

  • weight should be wrapped in a weighted_routing_policy {} block.
  • Added support for routing policies other than weighted (i.e. latency and geolocation)
  • Don't use duplicate resource names; differentiate similarly-named resource names with a counter.

mioi added 3 commits January 11, 2017 17:49
@mioi
add attributes for latency and geolocation based routing policies
cf8e0ca 
including additional attributes for latency and geolocation based
route53 routing policies.
@mioi
don't use duplicate resource names for aws_route53_records
4207655 
Terraform will complain if you use the same resource name for
aws_route53_records. Make some effort to make them unique by
appending a counter to the name.
@mioi
fix test
07c7ad8
dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
end

# TODO(dtan4): change method name...
def name_of(dns_name)
dns_name.gsub(/\.\z/, "")
end

def module_name_of(record)
normalize_module_name(name_of(record.name) + "-" + record.type)
def module_name_of(record, counter)
Copy link
Owner

Choose a reason for hiding this comment

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

[method_definition_validator]

Copy link
Author

Choose a reason for hiding this comment

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

It appears as though each resource type has its own module_name_of() method, so changing how the route53_record one works should be independent of the other ones.

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
record_sets_of(hosted_zone).map { |record| { record: record, zone_id: zone_id_of(hosted_zone) } }
end.flatten
count = {}
dups = to_return.group_by{ |record| module_name_of(record[:record], nil) }.select { |k, v| v.size > 1 }.map(&:first)
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Space missing to the left of {.
  • Unused block argument - k. If it's necessary, use _ or _k as an argument name to indicate that it won't be used. :ref

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
dups = to_return.group_by{ |record| module_name_of(record[:record], nil) }.select { |k, v| v.size > 1 }.map(&:first)
to_return.each do |r|
module_name = module_name_of(r[:record], nil)
if dups.include?(module_name)
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Use next to skip iteration. :ref

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
to_return.each do |r|
module_name = module_name_of(r[:record], nil)
if dups.include?(module_name)
if count[module_name]
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Use the return of the conditional for variable assignment and comparison.

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
module_name = module_name_of(r[:record], nil)
if dups.include?(module_name)
if count[module_name]
count[module_name] = count[module_name] + 1
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Use 2 (not -19) spaces for indentation. :ref

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
if dups.include?(module_name)
if count[module_name]
count[module_name] = count[module_name] + 1
else
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Align else with if.

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
if count[module_name]
count[module_name] = count[module_name] + 1
else
count[module_name] = 0
Copy link
Owner

Choose a reason for hiding this comment

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

[rubocop]

  • Use 2 (not -19) spaces for indentation. :ref

dtan4
dtan4 reviewed Jan 12, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
end

# TODO(dtan4): change method name...
def name_of(dns_name)
dns_name.gsub(/\.\z/, "")
end

def module_name_of(record)
normalize_module_name(name_of(record.name) + "-" + record.type)
def module_name_of(record, counter)
Copy link
Owner

Choose a reason for hiding this comment

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

[method_definition_validator]

@mioi mioi changed the title couple of fixes for route53_records Fixes for route53_records Jan 12, 2017
dtan4
dtan4 suggested changes Jan 15, 2017
View reviewed changes
spec/lib/terraforming/resource/route53_record_spec.rb
@@ -108,7 +108,9 @@ module Resource
zone_id = "OPQRSTUVWXYZAB"
name = "www.fuga.net"
type = "A"
weight = 10
weighted_routing_policy {
weight = 10
Copy link
Owner

Choose a reason for hiding this comment

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

Please use 4-spaces indention, like alias.name below.

lib/terraforming/resource/route53_record.rb
attributes["continent"] = record.geo_location.continent_code if record.geo_location.continent_code
attributes["country"] = record.geo_location.country_code if record.geo_location.country_code
attributes["subdivision"] = record.geo_location.subdivision_code if record.geo_location.subdivision_code
end
Copy link
Owner

Choose a reason for hiding this comment

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

Please add empty lines before if and after end.

attributes["region"] = record.region if record.region

if record.geo_location
# ...
end

attributes["set_identifier"] = record.set_identifier if record.set_identifier

@dtan4
Copy link
Owner

dtan4 commented Jan 15, 2017

Is there any case where the resource names were duplicated?

@Yasumoto
Copy link

Thanks for adding this @mioi !

@mioi
Fixups
3031af9 
* Fixed indentation
* Fixed padding of if..end block
dtan4
dtan4 reviewed Jan 16, 2017
View reviewed changes
lib/terraforming/resource/route53_record.rb
end

# TODO(dtan4): change method name...
def name_of(dns_name)
dns_name.gsub(/\.\z/, "")
end

def module_name_of(record)
normalize_module_name(name_of(record.name) + "-" + record.type)
def module_name_of(record, counter)
Copy link
Owner

Choose a reason for hiding this comment

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

[method_definition_validator]

@mioi
Copy link
Author

mioi commented Jan 16, 2017

@dtan4 in response to "Is there any case where the resource names were duplicated?":

Yes. When there are Route53 records with the same name and type, the resource name will be the same. A set of records' names and types would be identical for things like weighted, latency, or geolocation based routing policies.

@dtan4
Copy link
Owner

dtan4 commented Jan 20, 2017

A set of records' names and types would be identical for things like weighted, latency, or geolocation based routing policies.

Thanks, I see.

LGTM 👍 Thanks ❗️

@dtan4 dtan4 merged commit 98b307e into dtan4:master Jan 20, 2017
@dtan4
Copy link
Owner

dtan4 commented Jan 22, 2017

Released as v0.13.1.

@dtan4 dtan4 mentioned this pull request Apr 10, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dtan4 dtan4 dtan4 requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@mioi @dtan4 @Yasumoto