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.

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

feat: Added full_override_name override to disable adding domain name to record names #69

Conversation

dmitriy-lukyanchikov
Copy link
Contributor

…efix to the record name

Description

add full_override_name override to disable adding domain name as a prefix to the record name

Motivation and Context

add possibility to read the exported dns file taht contains full names.

Breaking Changes

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@dmitriy-lukyanchikov dmitriy-lukyanchikov changed the title add full_override_name override to disable adding domain name as a pr… feat: add full_override_name override to disable adding domain name as a pr… May 17, 2022
@dmitriy-lukyanchikov dmitriy-lukyanchikov changed the title feat: add full_override_name override to disable adding domain name as a pr… feat: Add full_override_name override to disable adding domain name as a pr… May 17, 2022
@antonbabenko
Copy link
Member

I would rather ask a user to prepare the valid records instead of adding a global flag. This is a bit unnecessary for this module.

@dmitriy-lukyanchikov
Copy link
Contributor Author

I would rather ask a user to prepare the valid records instead of adding a global flag. This is a bit unnecessary for this module.

maybe we can add per record flag? I believe preparing the records means using some regexp to actually edit records, i think the way that is used in this module is unnecessary because terraform provider and aws understand both jsut names without domain and full domain name so there is no need to prepend the domain name at all right now so current implementation is a bit strange. But maybe i miss some logic here

@antonbabenko
Copy link
Member

After small thinking, I think that having this flag on a per-record basis and making it backward-compatible may be a good option. Please make the changes in the example to show both options.

@antonbabenko antonbabenko reopened this May 18, 2022
@dmitriy-lukyanchikov
Copy link
Contributor Author

After small thinking, I think that having this flag on a per-record basis and making it backward-compatible may be a good option. Please make the changes in the example to show both options.

OK, will do, thank you for understanding!

@dmitriy-lukyanchikov
Copy link
Contributor Author

ok it is done

@dmitriy-lukyanchikov
Copy link
Contributor Author

@antonbabenko can somebody review, please?

@antonbabenko
Copy link
Member

Yes, I will do it later this week. Too busy with countless other tasks at the moment.

@antonbabenko antonbabenko changed the title feat: Add full_override_name override to disable adding domain name as a pr… feat: Added full_override_name override to disable adding domain name to record names May 27, 2022
@antonbabenko antonbabenko merged commit 553a4e1 into terraform-aws-modules:master May 27, 2022
antonbabenko pushed a commit that referenced this pull request May 27, 2022
## [2.7.0](v2.6.0...v2.7.0) (2022-05-27)

### Features

* Added full_override_name override to disable adding domain name to record names ([#69](#69)) ([553a4e1](553a4e1))
@antonbabenko
Copy link
Member

This PR is included in version 2.7.0 🎉

@antonbabenko
Copy link
Member

I have updated the example a bit.

It is working but there is still issue #59 that prevents examples from being 100% working.

@dmitriy-lukyanchikov
Copy link
Contributor Author

probably to fix it completely need to rework it completely and remove data resources at all. I can do it if you want

@antonbabenko
Copy link
Member

You can try. Keep in mind backward compatibility or at least a decent migration path for existing use-cases for terraform and terragrunt users.

@dmitriy-lukyanchikov
Copy link
Contributor Author

You can try. Keep in mind backward compatibility or at least a decent migration path for existing use-cases for terraform and terragrunt users.

ok will try

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
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.

None yet

2 participants