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

don't fail if parent_zone_name is not provided #51

Merged
merged 14 commits into from
Jun 30, 2022
Merged

don't fail if parent_zone_name is not provided #51

merged 14 commits into from
Jun 30, 2022

Conversation

kevcube
Copy link
Contributor

@kevcube kevcube commented Feb 18, 2022 •
edited
Loading

what

why

  • to use the module with a parent zone that is managed elsewhere,
  parent_zone_record_enabled = false
  zone_name                  = "$${stage}.example.com

INSTEAD OF...

  parent_zone_name           = "example.com"
  parent_zone_record_enabled = false
  zone_name                  = "$${stage}.$${parent_zone_name}"

alternatives considered

  • main.tf:25
    "$${parent_zone_name}", coalesce(join("", data.aws_route53_zone.parent_zone.*.name), var.parent_zone_name, "no_parent_zone_name")),
  • I consider removing one of var.parent_zone_id or var.parent_zone_name because offering both can lead to conflict or confusion. If var.parent_zone_id is removed, then we can always rely on var.parent_zone_name instead of coalescing with the output of the data.aws_route53_zone.parent_zone

@kevcube kevcube requested review from a team as code owners February 18, 2022 21:55
korenyoni
korenyoni previously requested changes Feb 22, 2022
View reviewed changes
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

This changeset results in route53 zones such as:

  # module.domain.aws_route53_zone.default[0] will be created
  + resource "aws_route53_zone" "default" {
      + arn           = (known after apply)
      + comment       = "Managed by Terraform"
      + force_destroy = false
      + id            = (known after apply)
      + name          = "test-domain.no_parent_zone_name"

We don't want the trailing .no_parent_zone_name.

Please see suggestions.

Also, please add another test for no parent zone.

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@korenyoni korenyoni mentioned this pull request Feb 22, 2022
Merged
@kevcube kevcube requested review from a team as code owners February 22, 2022 19:20
@mergify mergify bot dismissed korenyoni’s stale review February 22, 2022 19:20

This Pull Request has been updated, so we're dismissing all reviews.

@kevcube
Copy link
Contributor Author

kevcube commented Feb 22, 2022

I'll be able to add another test sometime this week.

@mergify
Copy link

mergify bot commented May 1, 2022

This pull request is now in conflict. Could you fix it @kevcube? 🙏

@kevcube kevcube force-pushed the work_without_parent_zone_name branch from 89ed1e9 to 0e4597d Compare June 23, 2022 22:10
@kevcube
Copy link
Contributor Author

kevcube commented Jun 23, 2022

@korenyoni test added, please re-review

aknysh
aknysh reviewed Jun 23, 2022
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@kevcube thank you, please see comments

aknysh
aknysh reviewed Jun 23, 2022
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@kevcube thank you, please see comments

@kevcube
Copy link
Contributor Author

kevcube commented Jun 23, 2022

@aknysh done!

aknysh
aknysh reviewed Jun 23, 2022
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@kevcube sorry, here as well

https://github.com/cloudposse/terraform-aws-route53-cluster-zone/blob/master/test/src/examples_complete_test.go#L19

@kevcube
Copy link
Contributor Author

kevcube commented Jun 23, 2022

yep @aknysh saw that one too. I added a new terra test file for this new example.

@aknysh
Copy link
Member

aknysh commented Jun 23, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Jun 23, 2022

@kevcube please see this error https://github.com/cloudposse/actions/runs/7033187015?check_suite_focus=true

@kevcube
Copy link
Contributor Author

kevcube commented Jun 24, 2022

@aknysh I think I've fixed it

@aknysh
Copy link
Member

aknysh commented Jun 27, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Jun 28, 2022

@kevcube please review the error https://github.com/cloudposse/actions/runs/7083410947?check_suite_focus=true, thank you

@kevcube
Copy link
Contributor Author

kevcube commented Jun 28, 2022

@aknysh 🤦

that one's fixed now!

@aknysh
Copy link
Member

aknysh commented Jun 28, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Jun 28, 2022

/test all

@kevcube
Copy link
Contributor Author

kevcube commented Jun 30, 2022

@aknysh I think it's fixed now.

@kevcube
Copy link
Contributor Author

kevcube commented Jun 30, 2022

/test all

2 similar comments
@Gowiem
Copy link
Member

Gowiem commented Jun 30, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Jun 30, 2022

/test all

@aknysh aknysh added the patch A minor, backward compatible change label Jun 30, 2022
@aknysh aknysh merged commit 7afff1d into cloudposse:master Jun 30, 2022
jbcom added a commit to jbcom/terraform-aws-route53-cluster-zone that referenced this pull request Nov 18, 2022
@kevcube @jbcom
@korenyoni @cloudpossebot
don't fail if parent_zone_name is not provided (cloudposse#51)
52a8e39 
* don't fail if parent_zone_name is not provided

* Apply suggestions from code review

* Reconciled with latest master

Co-authored-by: Jon Bogaty <[email protected]>
Co-authored-by: Yonatan Koren <[email protected]>

* PR feedback

* add test with no parent zone

* make number from bools

* Auto Format

* match main

* change region

* upd region

* actually add tests

* fix bool cast

* fix required var

* parent_zone record false

* fix example

Co-authored-by: Jon Bogaty <[email protected]>
Co-authored-by: Yonatan Koren <[email protected]>
Co-authored-by: cloudpossebot <[email protected]>
jbcom added a commit to jbcom/terraform-aws-route53-cluster-zone that referenced this pull request Nov 18, 2022
@kevcube @jbcom
@korenyoni @cloudpossebot
don't fail if parent_zone_name is not provided (cloudposse#51)
6b5fdec 
* don't fail if parent_zone_name is not provided

* Apply suggestions from code review

* Reconciled with latest master

Co-authored-by: Jon Bogaty <[email protected]>
Co-authored-by: Yonatan Koren <[email protected]>

* PR feedback

* add test with no parent zone

* make number from bools

* Auto Format

* match main

* change region

* upd region

* actually add tests

* fix bool cast

* fix required var

* parent_zone record false

* fix example

Co-authored-by: Jon Bogaty <[email protected]>
Co-authored-by: Yonatan Koren <[email protected]>
Co-authored-by: cloudpossebot <[email protected]>
@kevcube kevcube deleted the work_without_parent_zone_name branch November 5, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknysh aknysh aknysh approved these changes

@korenyoni korenyoni korenyoni left review comments

@Makeshift Makeshift Awaiting requested review from Makeshift Makeshift was automatically assigned from cloudposse/contributors

@RothAndrew RothAndrew Awaiting requested review from RothAndrew RothAndrew was automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
patch A minor, backward compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kevcube @aknysh @Gowiem @korenyoni @cloudpossebot