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

Modified 'aci_rest' and 'aci_config_snapshot' modules to display the correct URL output string #487

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

shrsr
Copy link
Collaborator

@shrsr shrsr commented Oct 3, 2023

No description provided.

@shrsr shrsr self-assigned this Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4cee5c5) 96.62% compared to head (e011f2c) 96.52%.
Report is 1 commits behind head on master.

❗ Current head e011f2c differs from pull request most recent head 4fb81a8. Consider uploading reports for the commit 4fb81a8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
- Coverage   96.62%   96.52%   -0.11%     
==========================================
  Files         183      183              
  Lines        8475     8478       +3     
  Branches     1265     1266       +1     
==========================================
- Hits         8189     8183       -6     
- Misses        214      222       +8     
- Partials       72       73       +1     
Flag Coverage Δ
integration 94.53% <66.66%> (-0.11%) ⬇️
sanity 35.17% <16.66%> (-0.01%) ⬇️

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

Files Coverage Δ
...ci_access_port_to_interface_policy_leaf_profile.py 98.46% <ø> (ø)
plugins/modules/aci_config_snapshot.py 100.00% <ø> (ø)
plugins/modules/aci_rest.py 92.30% <66.66%> (-1.90%) ⬇️

... and 2 files with indirect coverage changes

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

@@ -85,7 +85,7 @@
HAS_XMLJSON_COBRA = False

try:
from ansible.module_utils.six.moves.urllib.parse import urlparse
from ansible.module_utils.six.moves.urllib.parse import urlparse, urlunparse
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this function in aci.py or should we place in the aci_rest which is the only place where it is used?

@@ -434,6 +438,9 @@ def main():
aci.result["totalCount"] = aci.totalCount

else:
# NOTE A case when aci_rest is used with check mode and the apic host is used directly from the inventory
if aci.connection is not None and aci.params.get("host") is None:
aci.url = replace_apic_host(aci.url, re.sub(r"[[\]]", "", aci.connection.get_option("host")).split(",")[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the only location where this is used because it is an exception we could also decide to only set here by leveraging the url parse/unparse function here

Suggested change
aci.url = replace_apic_host(aci.url, re.sub(r"[[\]]", "", aci.connection.get_option("host")).split(",")[0])
aci.url = urlunparse(urlparse(aci.url)._replace(netloc=re.sub(r"[[\]]", "", aci.connection.get_option("host")).split(",")[0]))

@shrsr shrsr requested a review from akinross October 4, 2023 19:25
Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lhercot lhercot left a comment

Choose a reason for hiding this comment

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

LGTM

@lhercot lhercot merged commit 84123ec into CiscoDevNet:master Oct 5, 2023
14 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants