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

ec2_snapshot_info add option for paginated requests #321

Merged
merged 12 commits into from
Apr 14, 2021

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Apr 9, 2021

SUMMARY

add new options max_results and next_token to read snapshot in paginated mode

#312

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_snapshot_info

ADDITIONAL INFORMATION

the following task will limit the number of snapshot returned in output

- name: limit snapshot to 50
  ec2_snapshot_info:
     max_results: 50
  register: snapshot_result

- name: read next 100 elements
  ec2_snapshot_info:
     max_results: 50
     next_token: "{{ snapshot_result.next_token }}"


abikouo and others added 2 commits April 9, 2021 17:09
…n_parameters.yaml to 321-ec2_snapshot_info-add_max_results_and_next_token_parameters.yaml
@ansibullbot
Copy link

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Apr 9, 2021
data_encryption_key_id:
description: The data encryption key identifier for the snapshot. This value is a unique identifier that \
corresponds to the data encryption key that was used to encrypt the original volume or snapshot copy.
snapshots:
Copy link
Contributor

Choose a reason for hiding this comment

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

For anyone else who sees this: this PR didn't change the results here, it's been this way since at least 2.9

Given support for 2.8 is about to be dropped I don't think it's worth adding a shim layer

plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
Co-authored-by: Mark Chappell <mchappel@redhat.com>
@abikouo abikouo requested a review from tremble April 9, 2021 15:55
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Apr 9, 2021
@goneri goneri requested review from jillr and goneri April 9, 2021 18:37
@@ -217,6 +217,61 @@
that:
- info_result.snapshots| length == 3

# check that snapshot_ids and max_results are mutually exclusive
- name: Get info about all snapshots in paginated mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

The names of the new tasks should reflect what they are testing - in CI results we won't be able to see the code comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The names of the new tasks should reflect what they are testing - in CI results we won't be able to see the code comments.

sure, I will update it
Thanks

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.

Does it make sense to also require max_tokens to be set if next_token_id is set (so you can't set next_token_id if max_tokens isn't set)?

plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
abikouo and others added 2 commits April 12, 2021 14:26
Co-authored-by: Mark Chappell <mchappel@redhat.com>
Co-authored-by: Mark Chappell <mchappel@redhat.com>
@abikouo
Copy link
Contributor Author

abikouo commented Apr 12, 2021

Does it make sense to also require max_tokens to be set if next_token_id is set (so you can't set next_token_id if max_tokens isn't set)?

there are independent parameters.
if next_token_id is set and max_results is not set the max number of results will be 1000
when max_results is set alone, it is a way to start from the begining.

@abikouo abikouo requested review from jillr and tremble April 12, 2021 12:55
plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
plugins/modules/ec2_snapshot_info.py Outdated Show resolved Hide resolved
@abikouo
Copy link
Contributor Author

abikouo commented Apr 14, 2021

@jillr could you please review and merge this if ok ? (integration tests have been updated as requested)

@tremble tremble added the gate label Apr 14, 2021
@ansible-zuul ansible-zuul bot merged commit 8011d3a into ansible-collections:main Apr 14, 2021
@abikouo abikouo deleted the 312 branch October 24, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants