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

Fix url parsing for collections with a name of "collections" #1376

Merged
merged 1 commit into from
Mar 4, 2023
Merged

Fix url parsing for collections with a name of "collections" #1376

merged 1 commit into from
Mar 4, 2023

Conversation

jctanner
Copy link
Contributor

@jctanner jctanner commented Feb 24, 2023 •
edited
Loading

https://issues.redhat.com/browse/AAH-2158
#1388

An alternative fix is to change

path("content/<path:distro_base_path>/collections/", include(v3_collection_urls))

to

path("content/<str:distro_base_path>/collections/", include(v3_collection_urls))

However, that will not allow for base paths containing "/"'s

@jctanner jctanner changed the title Add test for oracleansible.collections Fix url parsing for collections with a name of "collections" Feb 24, 2023
mdellweg
mdellweg reviewed Feb 27, 2023
View reviewed changes
pulp_ansible/app/urls.py Outdated Show resolved Hide resolved
mdellweg
mdellweg reviewed Feb 27, 2023
View reviewed changes
pulp_ansible/app/urls.py Outdated Show resolved Hide resolved
@jctanner
Copy link
Contributor Author

@mdellweg I think the latest commit fixes it in a cleaner way. By being more specific about the route the detail urls are included on top of, the path logic seems to work better.

@jctanner jctanner requested a review from mdellweg March 1, 2023 20:20
mdellweg
mdellweg reviewed Mar 2, 2023
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Thank you for refining the tests. This is so much easier to review now.

pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Outdated Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Outdated Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Outdated Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Show resolved Hide resolved
pulp_ansible/tests/functional/api/collection/v3/test_collection_naming_edgecases.py Outdated Show resolved Hide resolved
mdellweg
mdellweg requested changes Mar 3, 2023
View reviewed changes
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Wait a second. There's an issue and a changelog missing.

@jctanner
Fix a bug for url parsing when the CV name is "collections".
a13b630 
fixes: pulp#1388

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@jctanner jctanner requested a review from mdellweg March 3, 2023 18:44
@mdellweg mdellweg merged commit 587382d into pulp:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdellweg mdellweg mdellweg approved these changes

@gerrod3 gerrod3 gerrod3 approved these changes

@newswangerd newswangerd Awaiting requested review from newswangerd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jctanner @mdellweg @gerrod3