-
Notifications
You must be signed in to change notification settings - Fork 54
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
Modernize functional tests III #1562
Conversation
21c3604
to
ac3387a
Compare
assert distribution.repository_version == repository.latest_version_href | ||
|
||
if False: | ||
# TODO This fails by complaining repository and version cannot be used together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a pulpcore bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7158c14
to
97d8a78
Compare
# TODO That's not how you get roles from galaxy v1... | ||
assert pulp_hash is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you figured out how to get roles from galaxy v1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. And also this test was skipped before. So do you think it brings new value, or should we remove it altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think this test doesn't add much value. We already test that we can download roles in the cli test when we check we can install a role from Pulp. I would suggest removing it.
if has_pulp_plugin("pulpcore", min="3.30.1"): | ||
# On older versions, this fails complaining repository and version cannot be used together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of has_pulp_plugin
!
pulp_ansible/app/serializers.py
Outdated
@@ -358,6 +358,7 @@ class Meta: | |||
"pulp_href", | |||
"pulp_created", | |||
"base_path", | |||
"base_url", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really be exposing base_url
for an ansible distribution? client_url
is the url you are most likely to need and I think if we do expose it we need to add a feature log for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to remove it again and mark the dysfunctional test mentioned above skipped.
(Maybe we get back to the roles api eventually...)
[noissue]
Backport to 0.18: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply d5fb061 on top of patchback/backports/0.18/d5fb061f37cc42a0cea7df52f36347154a215c88/pr-1562 Backporting merged PR #1562 into main
🤖 @patchback |
[noissue]