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

route53_info: Add snake_cased return key,values and a deprecation message #1236

Conversation

mandar242
Copy link
Contributor

@mandar242 mandar242 commented Jun 14, 2022

Depends-On: ansible/ansible-zuul-jobs#1564

SUMMARY

Add snake_case return values and a deprecation message for existing CamelCase return values.

Route53_info currently returns CamelCase values, to have uniformity along all *_info modules in terms of return values, it should return snake_case values instead.

Proposed change should make addition of snake_case return values and the deprecation message provides time for users to upgrade their playbooks to avoid breaking existing playbooks due to the proposed change.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

route53_info

ADDITIONAL INFORMATION

This PR is relation to the initiative for having updated developer guidelines for *_info modules specifically related to guidelines for deprecating return values.

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) labels Jun 14, 2022
@softwarefactory-project-zuul

This comment was marked as off-topic.

@tremble
Copy link
Contributor

tremble commented Jun 14, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

ansible-galaxy-importer FAILURE in 4m 24s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 23s
✔️ ansible-test-sanity-docker-devel SUCCESS in 11m 24s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 11m 32s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 04s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 01s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 7m 21s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 44s
✔️ ansible-test-splitter SUCCESS in 2m 52s
✔️ integration-community.aws-1 SUCCESS in 7m 14s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

I'm good with this change. I've submitted ansible/ansible-zuul-jobs#1564 to drop the 2.9 Sanity tests as was discussed in the last Ansible AWS community meeting. Assuming we can get the tests dropped I don't think there's value in updating the ignore files for 4.x/3.x

@tremble
Copy link
Contributor

tremble commented Jun 14, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

ansible-galaxy-importer FAILURE in 5m 15s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 25s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 54s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 11m 49s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 23s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 11m 12s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 08s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 23s
✔️ ansible-test-splitter SUCCESS in 2m 31s
✔️ integration-community.aws-1 SUCCESS in 8m 21s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

Copy link
Contributor

@jatorcasso jatorcasso left a comment

Choose a reason for hiding this comment

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

This LGTM but we should document the RETURN data in this module to clear up whats now being returned. Also some integrations tests should be added somewhere but that might be in a different PR

Copy link
Contributor

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

LGTM! Agree with what @jatorcasso suggested!

@mandar242
Copy link
Contributor Author

This LGTM but we should document the RETURN data in this module to clear up whats now being returned. Also some integrations tests should be added somewhere but that might be in a different PR

yes, that makes sense. I am planning to open a different PR for adding RETURN data block once this PR is merged.
Probably would add integration tests with new return data in the same.

@jatorcasso jatorcasso added the backport-3 PR should be backported to the stable-3 branch label Jun 14, 2022
@mandar242 mandar242 added the mergeit Merge the PR (SoftwareFactory) label Jun 14, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

ansible-galaxy-importer FAILURE in 4m 12s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 45s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 11s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 44s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 26s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 21s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 42s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 57s
✔️ ansible-test-splitter SUCCESS in 2m 25s
✔️ integration-community.aws-1 SUCCESS in 6m 55s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 3df423a into ansible-collections:main Jun 14, 2022
@patchback
Copy link

patchback bot commented Jun 14, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/3df423acf692fa53db7f729c2cc5baf440d55229/pr-1236

Backported as #1237

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jun 14, 2022
…sage (#1236)

route53_info: Add snake_cased return key,values and a deprecation message

Depends-On: ansible/ansible-zuul-jobs#1564
SUMMARY

Add snake_case return values and a deprecation message for existing CamelCase return values.
Route53_info currently returns CamelCase values, to have uniformity along all *_info modules in terms of return values, it should return snake_case values instead.
Proposed change should make addition of snake_case return values and the deprecation message provides time for users to upgrade their playbooks to avoid breaking existing playbooks due to the proposed change.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

route53_info
ADDITIONAL INFORMATION

This PR is relation to the initiative for having updated developer guidelines for *_info modules specifically related to guidelines for deprecating return values.

Reviewed-by: Mark Chappell <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Alina Buzachis <None>
(cherry picked from commit 3df423a)
@github-actions
Copy link

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jun 14, 2022
…sage (#1236) (#1237)

[PR #1236/3df423ac backport][stable-3] route53_info: Add snake_cased return key,values and a deprecation message

This is a backport of PR #1236 as merged into main (3df423a).
Depends-On: ansible/ansible-zuul-jobs#1564
SUMMARY

Add snake_case return values and a deprecation message for existing CamelCase return values.
Route53_info currently returns CamelCase values, to have uniformity along all *_info modules in terms of return values, it should return snake_case values instead.
Proposed change should make addition of snake_case return values and the deprecation message provides time for users to upgrade their playbooks to avoid breaking existing playbooks due to the proposed change.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

route53_info
ADDITIONAL INFORMATION


This PR is relation to the initiative for having updated developer guidelines for *_info modules specifically related to guidelines for deprecating return values.

Reviewed-by: Mark Chappell <None>
@mandar242 mandar242 deleted the route53_info_add_snake_case_return_values branch June 14, 2022 18:50
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jun 17, 2022
route53_info: Add RETURN block

SUMMARY

Currently route53_info is mising a return block.
This is a follow up on #1236

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mike Graves <mgraves@redhat.com>
Reviewed-by: Mandar Kulkarni <mandar242@gmail.com>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jul 7, 2022
…thout updating existing health check (#1143) (#1324)

[PR #1143/8d3fec8e backport][stable-4] route53_health_check: Add feature to create multiple health checks without updating existing health check

This is a backport of PR #1143 as merged into main (8d3fec8).
SUMMARY

Might fix #88


Update [06/30/2022]:
This PR adds three new parameters to route53_health_check module.
health_check_id=dict(type='str', aliases=['id'], required=False),
health_check_name=dict(type='str', aliases=['name'], required=False),
use_unique_names=dict(type='bool', required=False),


health_check_id (alias id): when set, can be used to update and also delete health checks.
use_unique_names and health_check_name (alias name): these are used together to make use of health check name as a unique identifier to create/update/delete health check and also assign a name to the health check.


Update [05/20/2022]:
Based on discussion between 15 May to 19 May below, the approach of adding ignore_existing parameter has been discarded for the time being.
The progress on this WIP PR put on hold until route53_info return value issue gets resolved. #1236

Added parameter ignore_existing that allows to create multiple route53 healthchecks with similar parameters instead of updating existing healthcheck (example: create multiple route53 healthchecks with different paths to same server).
ISSUE TYPE


Feature Pull Request

COMPONENT NAME

route53_health_check
ADDITIONAL INFORMATION


Consider following example from the issue.

- name: set up route53 health checks
  connection: local
  become: false
  route53_health_check:
    failure_threshold: 3
    ip_address: '{{ ip_address }}'
    port: 80
    request_interval: 30
    resource_path: '{{ item }}'
    state: present
    type: HTTP
    #ignore_existing: True
  register: result
  with_items:
    - /some_status1
    - /other_status2

With ignore_exisitng parameter, it will create two healthchecks to same sever with different resource_path.
Without ignore_existing parameter, it will create first healthcheck to sever, then start creating the second healthcheck but as it's having same server/ip_address, it updates the existing healthcheck instead, so creates only single healthcheck at the end of the task.

Reviewed-by: Mark Chappell <None>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jul 7, 2022
…1322)

route53_info: Add snake_cased return key,values to remaining methods

SUMMARY

Following up on #1236
Found more places where route53_info module does not return a snake_case output.
Added snake_case output to checker_ip_range_details , reusable_delegation_set_details, and get_health_check methods.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
patchback bot pushed a commit that referenced this pull request Jul 7, 2022
…1322)

route53_info: Add snake_cased return key,values to remaining methods

SUMMARY

Following up on #1236
Found more places where route53_info module does not return a snake_case output.
Added snake_case output to checker_ip_range_details , reusable_delegation_set_details, and get_health_check methods.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
(cherry picked from commit d7f3862)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jul 8, 2022
…1322) (#1327)

[PR #1322/d7f38627 backport][stable-4] route53_info: Add snake_cased return key,values to remaining methods

This is a backport of PR #1322 as merged into main (d7f3862).
SUMMARY

Following up on #1236
Found more places where route53_info module does not return a snake_case output.
Added snake_case output to checker_ip_range_details , reusable_delegation_set_details, and get_health_check methods.

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…sage (ansible-collections#1236)

route53_info: Add snake_cased return key,values and a deprecation message

Depends-On: ansible/ansible-zuul-jobs#1564
SUMMARY

Add snake_case return values and a deprecation message for existing CamelCase return values.
Route53_info currently returns CamelCase values, to have uniformity along all *_info modules in terms of return values, it should return snake_case values instead.
Proposed change should make addition of snake_case return values and the deprecation message provides time for users to upgrade their playbooks to avoid breaking existing playbooks due to the proposed change.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

route53_info
ADDITIONAL INFORMATION

This PR is relation to the initiative for having updated developer guidelines for *_info modules specifically related to guidelines for deprecating return values.

Reviewed-by: Mark Chappell <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@3df423a
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
route53_info: Add RETURN block

SUMMARY

Currently route53_info is mising a return block.
This is a follow up on ansible-collections#1236

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mike Graves <mgraves@redhat.com>
Reviewed-by: Mandar Kulkarni <mandar242@gmail.com>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@6e7f150
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…nsible-collections#1322)

route53_info: Add snake_cased return key,values to remaining methods

SUMMARY

Following up on ansible-collections#1236
Found more places where route53_info module does not return a snake_case output.
Added snake_case output to checker_ip_range_details , reusable_delegation_set_details, and get_health_check methods.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

route53_info

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@d7f3862
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
ec2_eni_info/tests: add unit-tests

SUMMARY


Add the unit-test coverage of the ec2_eni_info module.
break up list_eni to move connection.describe_network_interfaces to separate method.
refactor ec2_eni_info module


COMPONENT NAME

ec2_eni_info

Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants