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(ipa_dnsrecord): add multiple record support #4578

Merged
merged 1 commit into from
May 9, 2022
Merged

feat(ipa_dnsrecord): add multiple record support #4578

merged 1 commit into from
May 9, 2022

Conversation

genofire
Copy link
Contributor

SUMMARY

Add the possibility to add multiple values under the same record

fixes #4553

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ipa_dnsrecord

ADDITIONAL INFORMATION

Should i rename record_value to record_values?
current i keep the state, so that it is not an breaking change - but i believe record_values would be more correct.

@ansibullbot
Copy link
Collaborator

cc @Akasurde @Nosmoht @fxfitz @justchris1
click here for bot help

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request identity module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Apr 26, 2022
@felixfontein
Copy link
Collaborator

Thanks for your contribution! Could you please add a changelog fragment?

I think it should have both record_value and record_values, with them being mutually exclusive. That way the module can be used as before without a breaking change with a single value, but also can be used as lists.

(The main problem is that if you change type: str to type: list, Ansible will split strings by comma to convert them to lists. This is a breaking change if it is possible to put strings with commas in there, which it is due to TXT record values.)

@genofire
Copy link
Contributor Author

Last changes with new records are tested automatically over night in your environment.

@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 27, 2022
@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Apr 29, 2022
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from 3e159c4 to 89b5677 Compare April 29, 2022 15:11
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from 969d102 to 3963c7a Compare April 29, 2022 15:28
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from 3963c7a to 5b2eec7 Compare April 29, 2022 15:39
@ansibullbot

This comment was marked as outdated.

Sign in to view
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from 5b2eec7 to c381c4f Compare April 29, 2022 16:35
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch 2 times, most recently from ca40759 to bcb75da Compare April 29, 2022 16:44
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from bcb75da to f77f358 Compare April 29, 2022 17:03
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 29, 2022
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from de2ef4b to 4f375e1 Compare April 29, 2022 21:27
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Apr 29, 2022
@genofire genofire requested a review from felixfontein May 2, 2022 10:09
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 6, 2022
@genofire @felixfontein
feat(ipa_dnsrecord): add multiple record support
f87c46d 
Co-authored-by: Felix Fontein <felix@fontein.de>
@genofire genofire force-pushed the feat/ipa_dnsrecord-multiple_recordvalues branch from 956c091 to f87c46d Compare May 6, 2022 14:50
@genofire genofire requested a review from felixfontein May 6, 2022 14:50
@ansibullbot ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 6, 2022
felixfontein
felixfontein approved these changes May 8, 2022
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks OK to me. I have no way to test this though, I would be glad if the module maintainers could weight in :)

@genofire
Copy link
Contributor Author

genofire commented May 8, 2022 •
edited
Loading

@Akasurde do you like to review / merge this PR ?

I am not able to set an reviewer here.

@justchris1
Copy link
Contributor

LGTM

@felixfontein felixfontein merged commit 72bffce into ansible-collections:main May 9, 2022
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label May 9, 2022
@felixfontein
Copy link
Collaborator

@genofire @justchris1 thanks for contributing this and reviewing!

@genofire genofire deleted the feat/ipa_dnsrecord-multiple_recordvalues branch May 12, 2022 16:12
@genofire
Copy link
Contributor Author

thanks for this good reviewing @felixfontein

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

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
feature This issue/PR relates to a feature request identity module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipa_dnsrecord: multiple record_value
4 participants
@genofire @ansibullbot @felixfontein @justchris1