-
Notifications
You must be signed in to change notification settings - Fork 35
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
Change the version check to use app label #341
Conversation
WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue. |
a5c66ba
to
86fc81f
Compare
Maybe I applied it wrong, but I think the failed when I tried to use it with pulpcore. See the failures here: pulp/pulpcore#1102 |
- name: "Check the component is in status" | ||
assert: | ||
that: | ||
- component_name in (result.json.versions | items2dict(key_name="component", value_name="version") | keys) |
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.
So the keys filter does not exist... still the rescue block is executed.
Then we should be fine just removing this task.
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 was so laser focused seeing a failure, that i saw a failure, not realizing it was the wrong one.
A change in pulpcore (status API) requires us to change the version test to use the app label instead of the repository name. Along with this change we introduce a legacy lookup as a fallback to not break all CI at the same time. [noissue]
86fc81f
to
0898f0b
Compare
@bmbouter Sorry, this version should work better. Please try again. And thank you for not blindly trusting it, but insisting to apply it to your PR first. |
I think it worked, but this time it's failing not on finding pulpcore, but on pulp_file (in the pulpcore CI). Any advice on what I should change to have that set the vars correctly too? I didn't expect it to check pulp_file also. https://github.com/pulp/pulpcore/pull/1102/checks?check_run_id=1926773090 |
That is not the check you fixed. But in the meantime the CLI tests were added to this CI. And the cli still uses the old names. |
Ah yes! Let's collab at our time tomorrow and figure out how to fix. Ty! |
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 successfully used this PR in my pulpcore PR, so I'm +1 to merge.
A change in pulpcore (status API) requires us to change the version test
to use the app label instead of the repository name.
Along with this change we introduce a legacy lookup as a fallback to not
break all CI at the same time.
[noissue]