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.

Sign up for GitHub

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

Jump to bottom

provider/aws: Add Route 53 delegation set resource #1999

Merged
merged 3 commits into from
Jun 23, 2015
Merged

provider/aws: Add Route 53 delegation set resource #1999

merged 3 commits into from
Jun 23, 2015

Conversation

radeksimko
Copy link
Member

Test plan (takes ~63mins to finish)

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=Route53' 2>/dev/null

go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=Route53 -timeout 90m
=== RUN TestAccRoute53DelegationSet
--- PASS: TestAccRoute53DelegationSet (2.15s)
=== RUN TestAccRoute53DelegationSet_withZones
--- PASS: TestAccRoute53DelegationSet_withZones (135.51s)
=== RUN TestAccRoute53Record_basic
--- PASS: TestAccRoute53Record_basic (273.96s)
=== RUN TestAccRoute53Record_txtSupport
--- PASS: TestAccRoute53Record_txtSupport (263.60s)
=== RUN TestAccRoute53Record_generatesSuffix
--- PASS: TestAccRoute53Record_generatesSuffix (273.69s)
=== RUN TestAccRoute53Record_wildcard
--- PASS: TestAccRoute53Record_wildcard (386.95s)
=== RUN TestAccRoute53Record_weighted
--- PASS: TestAccRoute53Record_weighted (266.26s)
=== RUN TestAccRoute53Record_alias
--- PASS: TestAccRoute53Record_alias (254.61s)
=== RUN TestAccRoute53Record_weighted_alias
--- PASS: TestAccRoute53Record_weighted_alias (529.06s)
=== RUN TestAccRoute53Record_TypeChange
--- PASS: TestAccRoute53Record_TypeChange (405.93s)
=== RUN TestAccRoute53ZoneAssociation_basic
--- PASS: TestAccRoute53ZoneAssociation_basic (281.50s)
=== RUN TestAccRoute53ZoneAssociation_region
--- PASS: TestAccRoute53ZoneAssociation_region (271.70s)
=== RUN TestAccRoute53Zone_basic
--- PASS: TestAccRoute53Zone_basic (153.32s)
=== RUN TestAccRoute53Zone_private_basic
--- PASS: TestAccRoute53Zone_private_basic (148.25s)
=== RUN TestAccRoute53Zone_private_region
--- PASS: TestAccRoute53Zone_private_region (147.78s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    3794.276s

See #2157

pmoust
pmoust reviewed May 18, 2015
View reviewed changes
website/Gemfile.lock
@@ -79,18 +79,18 @@ GEM
celluloid (~> 0.16.0)
rb-fsevent (>= 0.9.3)
rb-inotify (>= 0.9)
middleman (3.3.13)
middleman (3.3.12)
Copy link
Contributor

Choose a reason for hiding this comment

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

git add .? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's the question/issue ? :)

I have not been changing anything in the actual Gemfile, all these packages are resolved dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved this change into a separate PR - #2073 to make it easier for anyone to review.

@pmoust
Copy link
Contributor

pmoust commented May 18, 2015

Thanks!

@radeksimko radeksimko force-pushed the r53-delegation-set branch 5 times, most recently from 85d8618 to 6a58565 Compare May 30, 2015 21:12
@radeksimko
Copy link
Member Author

I just fixed a small bug w/ aws_route53_zone.delegation_set and I'm just getting to the bottom of name_servers list interpolation.

Either way, I'm relatively sure that the interpolation issue is a separate one and will be solved globally (without having to touch any specific resource/field/config), so this PR is ready for review & merge.

@radeksimko
Copy link
Member Author

btw. if anyone prefers aws_route53_zone.delegation_set_id over aws_route53_zone.delegation_set, then I'm okay with changing it, I'm just not sure what's the convention for this, but _id suffix actually makes more sense to me.

@phinze
Copy link
Contributor

phinze commented May 30, 2015

Yeah +1 for the _id suffix. Helps to steer people away from passing the whole resource in as the value for the attribute.

@radeksimko radeksimko mentioned this pull request May 30, 2015
Merged
4 tasks
@radeksimko radeksimko mentioned this pull request May 31, 2015
Merged
@radeksimko
Copy link
Member Author

Hold off on merging this - I'm waiting for successful run of all R53 acceptance tests and it's failing for some reason, not sure if it's just #2158 or something more serious.

@radeksimko radeksimko changed the title provider/aws: Add Route 53 delegation set resource [HOLD] provider/aws: Add Route 53 delegation set resource May 31, 2015
@radeksimko
Copy link
Member Author

This is ready for review & merge, I just successfully finished running all mentioned acceptance tests.

@radeksimko radeksimko changed the title [HOLD] provider/aws: Add Route 53 delegation set resource provider/aws: Add Route 53 delegation set resource Jun 1, 2015
catsby
catsby reviewed Jun 2, 2015
View reviewed changes
builtin/providers/aws/resource_aws_route53_delegation_set.go
input := &route53.CreateReusableDelegationSetInput{
CallerReference: aws.String(callerRef),
}
if v, ok := d.GetOk("zone_id"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

zone_id isn't in the schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I wonder how come this wasn't caught by any test... 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

So this was actually in the original concept, but then I trashed the idea of defining zone_id directly from this resource as I can't see any meaningful use-case for it.

There's aws_route53_zone.delegation_set_id which I'm adding and I believe the pairing will be much more useful that way.

So I guess I can just remove these three lines unless you can think of a use-case for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

unless you can think of a use-case for it.

I didn't see a use-case on my first pass of the code / API docs

@catsby
Copy link
Contributor

catsby commented Jun 2, 2015

2 nitpicks, otherwise looks great!

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Jun 2, 2015
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 2, 2015
@radeksimko radeksimko force-pushed the r53-delegation-set branch 4 times, most recently from 02b68ca to 83908e0 Compare June 3, 2015 21:38
@radeksimko
Copy link
Member Author

@catsby When you have some spare time, would you give this a final review, please? :)

@catsby
Copy link
Contributor

catsby commented Jun 23, 2015

LGTM, feel free to merge 👍

radeksimko added a commit that referenced this pull request Jun 23, 2015
@radeksimko
Merge pull request #1999 from TimeIncOSS/r53-delegation-set
5cf5451 
provider/aws: Add Route 53 delegation set resource
@radeksimko radeksimko merged commit 5cf5451 into hashicorp:master Jun 23, 2015
@radeksimko radeksimko deleted the r53-delegation-set branch June 23, 2015 21:51
@ghost
Copy link

ghost commented May 2, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement provider/aws
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@radeksimko @pmoust @phinze @catsby