Drop Pulpcore 3.14 & 3.15, move to 3.16 and 3.17 #249
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I just realized 3.17 states (https://docs.pulpproject.org/pulpcore/changes.html):
This still needs to be addressed in this PR. In particular with 3.16 implications. |
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
c3328af
to
b33b7e2
Compare
ekohl
commented
Mar 9, 2022
spec/acceptance/basic_spec.rb
Outdated
| @@ -65,8 +65,7 @@ | |||
| end | |||
|
|
|||
| describe curl_command("https://#{host_inventory['fqdn']}/pulp/api/v3/", cacert: "#{certdir}/ca-cert.pem") do | |||
| its(:response_code) { is_expected.to eq(200) } | |||
| its(:body) { is_expected.not_to contain('artifacts_list') } | |||
| its(:response_code) { is_expected.to eq(403) } | |||
There was a problem hiding this comment.
I do wonder if this is correct. Also, why is the HTTP 200 code in the other tests (plugin, content route spec).
There was a problem hiding this comment.
On the other hand, what it this supposed to be? The api root, listing all the endpoints? Does it work if you pass a cert/key?
There was a problem hiding this comment.
So what I do find odd is that it does pass in the other tests. I haven't looked into the details yet, but the inconsistency strikes me as odd.
There was a problem hiding this comment.
I think it's a side-effect of pulp/pulpcore#1593 (and I've asked in pulp-dev to make sure).
If you want to test an endpoint w/o auth, use /pulp/api/v3/status/ as we do in line 57.
That said, I am not really sure what this test is supposed to test. It was added as part of a60df53 where we implemented HTTPS handling inside this module, but it isn't saying what it's testing :(
There was a problem hiding this comment.
FTR, I've opened pulp/pulpcore#2340 as Grant said on IRC this was probably not an intentional change.
I do however stand by my analysis that this test as such is not giving any value to this module (while the /status one does) and can be dropped.
There was a problem hiding this comment.
The idea was mostly that it showed a difference between authenticated and non-authenticated. With that you can see that certificate authentication works. That's something this module sets up. If you have a better idea of how to test auth, I'm open to suggestions.
There was a problem hiding this comment.
But that's exactly what it did not test, as access to / was explicitly allowed without auth (just the content differed).
I would suggest hitting a real API endpoint like /users or /artifacts or /tasks. Without auth you get status=403 and a JSON reply of {"detail":"Authentication credentials were provided."}, with you get status=200 and some actual content.
There was a problem hiding this comment.
But that's exactly what it did not test, as access to
/was explicitly allowed without auth (just the content differed).
I disagree it did not test it. By checking the difference in output I believe I was able to test it. You may be right that it happened to be a side effect. I'd be happy to merge a follow up PR.
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
b33b7e2
to
3dc23d8
Compare
|
The failures look like bugs in Pulp 3.17: |
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
3dc23d8
to
c9e0272
Compare
|
Updated the PR to only 3.16 since 3.17 is broken on EL7. |
ehelms
changed the title
ehelms
reviewed
Mar 10, 2022
|
Note to self: include enabling the pulpcore el8 module in |
evgeni
approved these changes
Mar 16, 2022
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
c9e0272
to
0b6f2d4
Compare
ehelms
reviewed
Mar 16, 2022
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
3 times, most recently
from
eb6a9cd
to
8072d6a
Compare
evgeni
reviewed
Mar 17, 2022
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
8072d6a
to
574cb60
Compare
evgeni
reviewed
Mar 17, 2022
manifests/repo.pp
Outdated
| @@ -19,6 +20,17 @@ | |||
| notify => Anchor['pulpcore::repo'], | |||
| } | |||
|
|
|||
| if $facts['os']['release']['major'] == '8' { | |||
There was a problem hiding this comment.
Suggested change
| if $facts['os']['release']['major'] == '8' { | |
| if $facts['os']['release']['major'] != '7' { |
to match tests? (and the idea of future proofing) :)
There was a problem hiding this comment.
I copied it from here: https://github.com/theforeman/puppet-foreman/blob/9e483426c511e7a7a2648c2eaba18f50ac7df8f9/manifests/repo.pp#L44
But you're right.
This will probably fix for 3.17 theforeman/pulpcore-packaging#418 |
It did, thanks! |
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
574cb60
to
aa7302a
Compare
|
Updated:
|
evgeni
reviewed
Mar 18, 2022
ekohl
force-pushed
the
update-to-3.16-and-3.17
branch
from
aa7302a
to
3dd9f37
Compare
evgeni
approved these changes
Mar 18, 2022
ehelms
approved these changes
Mar 21, 2022
ehelms
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.