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

feat: dns records and zones inclusion #910

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

feat: dns records and zones inclusion #910

wants to merge 22 commits into from
+417 −1

Conversation

imprateeksh
Copy link
Member

@imprateeksh imprateeksh commented Jan 7, 2025 •
edited
Loading

Description

This PR is to add the change required to include DNS records and zones.
Refer Internal Issue: #11726

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content
  • Added support for DNS zone and records management.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@imprateeksh imprateeksh self-assigned this Jan 7, 2025
@imprateeksh imprateeksh changed the title [Do not merge] feat: dns records and zones inclusion feat: dns records and zones inclusion Feb 4, 2025
@imprateeksh
Copy link
Member Author

Moved the test to other_tests.go
Test passed successfully. Logs - logs-vpc-with-dns.txt

@imprateeksh
Copy link
Member Author

/run pipeline

@imprateeksh imprateeksh marked this pull request as ready for review February 4, 2025 23:46
@imprateeksh
Copy link
Member Author

/run pipeline

@imprateeksh
Copy link
Member Author

/run pipeline

@imprateeksh
Copy link
Member Author

/run pipeline

@imprateeksh
Copy link
Member Author

/run pipeline

rajatagarwal-ibm
rajatagarwal-ibm requested changes Feb 10, 2025
View reviewed changes
Copy link
Member

@rajatagarwal-ibm rajatagarwal-ibm left a comment

Choose a reason for hiding this comment

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

Have you run the DNS tests? If yes, how did they turn out?

variables.tf Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf
}))
default = []
validation {
condition = length(var.dns_records) == 0 || alltrue([for record in var.dns_records != null ? var.dns_records : [] : (contains(["A", "AAAA", "CNAME", "MX", "PTR", "TXT", "SRV"], record.type))])
Copy link
Member

Choose a reason for hiding this comment

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

I am uncertain if we need all types of dns_records as it will be consumed in landing zone, can you please confirm this @vburckhardt?

Copy link
Member Author

Choose a reason for hiding this comment

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

This change was introduced as part of HPC requirement, this shall be consumed in SLZ too. If some fields are to be omitted kindly suggest else we can go ahead with all the DNS records.

outputs.tf Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
examples/vpc-with-dns/README.md Outdated Show resolved Hide resolved
examples/vpc-with-dns/variables.tf Outdated Show resolved Hide resolved
examples/vpc-with-dns/variables.tf Show resolved Hide resolved
examples/vpc-with-dns/variables.tf Show resolved Hide resolved
examples/vpc-with-dns/outputs.tf Outdated Show resolved Hide resolved
@imprateeksh
Copy link
Member Author

Have you run the DNS tests? If yes, how did they turn out?

Yes, I tried testing all DNS records and it worked fine. Also tried provisioning before running the test.
I had already shared the test results.

@imprateeksh
Copy link
Member Author

/run pipeline

rajatagarwal-ibm
rajatagarwal-ibm reviewed Feb 11, 2025
View reviewed changes
main.tf Outdated Show resolved Hide resolved
outputs.tf Show resolved Hide resolved
@imprateeksh
Copy link
Member Author

Hi @rajatagarwal-ibm - I have a query around dns-zone - Do you suggest to include a validation for this or we will let provider handle it as this is not mentioned in the terraform provider doc ?

@rajatagarwal-ibm
Copy link
Member

rajatagarwal-ibm commented Feb 13, 2025 •
edited
Loading

@imprateeksh I think it will be good to have a validation around dns-zone for subdomains, but not necessary.

Maybe you could use contains to validate that.

@imprateeksh
Copy link
Member Author

/run pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maheshwarishikha maheshwarishikha Awaiting requested review from maheshwarishikha maheshwarishikha is a code owner

@rajatagarwal-ibm rajatagarwal-ibm Awaiting requested review from rajatagarwal-ibm rajatagarwal-ibm is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@imprateeksh imprateeksh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@imprateeksh @rajatagarwal-ibm