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

[minor_change] Add pagination support for aci_rest module (DCNE-101) #711

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

edudppaz
Copy link
Contributor

This change adds "page_size" and "page" optional paremeters to the aci_rest module in order to support pagination.

if the page_size parameter is set on a GET request, it controls the amount of objects to be returned on each page by the APIC response, then the page parameter controls which page is returned to the module.
If the page parameter is not set, it returns the first page by default (0)

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 35.72%. Comparing base (7272ad6) to head (e8bf63a).

Files with missing lines Patch % Lines
plugins/modules/aci_rest.py 0.00% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7272ad6) and HEAD (e8bf63a). Click for more details.

HEAD has 14 uploads less than BASE
Flag BASE (7272ad6) HEAD (e8bf63a)
integration 2 0
unit 8 0
sanity 8 4
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #711       +/-   ##
===========================================
- Coverage   95.67%   35.72%   -59.95%     
===========================================
  Files         270      267        -3     
  Lines       12405    12225      -180     
  Branches     1874     1849       -25     
===========================================
- Hits        11868     4367     -7501     
- Misses        406     7858     +7452     
+ Partials      131        0      -131     
Flag Coverage Δ
integration ?
sanity 35.72% <0.00%> (-0.03%) ⬇️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akinross akinross added the jira-sync Sync this issue to Jira label Dec 16, 2024
@github-actions github-actions bot changed the title [minor_change] Add pagination support for aci_rest module [minor_change] Add pagination support for aci_rest module (DCNE-268) Dec 16, 2024
@akinross akinross changed the title [minor_change] Add pagination support for aci_rest module (DCNE-268) [minor_change] Add pagination support for aci_rest module (DCNE-101) Dec 16, 2024
@akinross akinross changed the title [minor_change] Add pagination support for aci_rest module (DCNE-101) [minor_change] Add pagination support for aci_rest module (DCNE-268) Dec 16, 2024
@akinross akinross linked an issue Dec 16, 2024 that may be closed by this pull request
@github-actions github-actions bot changed the title [minor_change] Add pagination support for aci_rest module (DCNE-268) [minor_change] Add pagination support for aci_rest module (DCNE-101) Dec 16, 2024
plugins/modules/aci_rest.py Outdated Show resolved Hide resolved
plugins/modules/aci_rest.py Outdated Show resolved Hide resolved
plugins/modules/aci_rest.py Outdated Show resolved Hide resolved
plugins/modules/aci_rest.py Outdated Show resolved Hide resolved
Comment on lines 475 to 476
aci.path = "{0}?page={1}&page-size={2}".format(aci.path, page, page_size)
aci.url = update_qsl(aci.url, {"page": page, "page-size": page_size})
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be a issue when doing setting url and path in this way when path contains other filters. As an example if path is "/api/class/fvSubnet.json?order-by=fvSubnet.descr" and you would add page information as above it would display the url as "https://173.36.219.82/api/class/fvSubnet.json?order-by=fvSubnet.descr?page=0&page-size=3" but would sent correctly to https://173.36.219.82:443/api/class/fvSubnet.json?order-by=fvSubnet.descr&page=0&page-size=3

as temp workaround tested that leveraging the update_qsl() function will display it correctly

Suggested change
aci.path = "{0}?page={1}&page-size={2}".format(aci.path, page, page_size)
aci.url = update_qsl(aci.url, {"page": page, "page-size": page_size})
aci.path = update_qsl(aci.path, {"page": page, "page-size": page_size})
aci.url = update_qsl(aci.url, {"page": page, "page-size": page_size})

I do think however we should rootcause this, because it is likely that similar behaviour is present for rsp-subtree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I´ll look into this. thanks.

tests/integration/targets/aci_rest/tasks/json_inline.yml Outdated Show resolved Hide resolved
plugins/modules/aci_rest.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync Sync this issue to Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aci_rest needs to support pagination (DCNE-101)
2 participants