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

Replace usage of _pulp_client with pubtools-pulplib [RHELDST-141] #223

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

negillett
Copy link
Member

This commit removes the library's native _pulp_client ("legacy") and replaces it with usage of pubtools-pulplib methods for associating and unassociating content.

Additionally, PulpActions were replaced with simple abstractions for grouping content to be associated/unassociated.

Tests were removed/edited as necessary.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (13ab604) to head (8455d0c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   98.85%   98.78%   -0.08%     
==========================================
  Files           8        7       -1     
  Lines         960      820     -140     
==========================================
- Hits          949      810     -139     
+ Misses         11       10       -1     

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

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett negillett force-pushed the 141-b branch 2 times, most recently from 397783e to a210e59 Compare March 14, 2024 19:55
@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett negillett force-pushed the 141-b branch 3 times, most recently from cc67272 to ce9bb0c Compare March 15, 2024 21:23
@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

negillett commented Mar 15, 2024

I'm at a total loss as to why we're seeing error 111 on '/api/v1/manifest'. I've touched nothing in the ubi client.
Any ideas, @rbikar?

@rbikar
Copy link
Member

rbikar commented Mar 18, 2024

run tests

@rbikar
Copy link
Member

rbikar commented Mar 18, 2024

I'm at a total loss as to why we're seeing error 111 on '/api/v1/manifest'. I've touched nothing in the ubi client. Any ideas, @rbikar?

I think the ubi-manifest service in the test env was in a bad shape, I re-ran tests and they passed OK.

ubipop/__init__.py Outdated Show resolved Hide resolved
ubipop/__init__.py Outdated Show resolved Hide resolved
ubipop/__init__.py Outdated Show resolved Hide resolved
ubipop/__init__.py Outdated Show resolved Hide resolved
tests/test_ubipop.py Outdated Show resolved Hide resolved
ubipop/__init__.py Outdated Show resolved Hide resolved
@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett negillett force-pushed the 141-b branch 2 times, most recently from ff424a7 to 674fa05 Compare March 26, 2024 21:31
@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

@rbikar, @rohanpm, this is ready for review.
I'm still unsure why some modulemd profiles no longer contain "[d]" to signal default but I don't believe the change was introduced here (it was only ever present in the integration tests). It could be a difference between rhel7 and rhel8 content--this is a key difference between the two integration tests that asserted the presence of the mark.

@rbikar
Copy link
Member

rbikar commented Mar 28, 2024

@rbikar, @rohanpm, this is ready for review. I'm still unsure why some modulemd profiles no longer contain "[d]" to signal default but I don't believe the change was introduced here (it was only ever present in the integration tests). It could be a difference between rhel7 and rhel8 content--this is a key difference between the two integration tests that asserted the presence of the mark.

In realistic usage the ubi7 shouldn't have any modules (but I guess in test the situation can be different).

Anyway you need to examine modulemd_defaults units in the test instance of rhsm-pulp before and after the test.

I checked current real ubi8 image and e.g. for perl module I get this:
Output of dnf module list

perl                 5.24       common [d Practical Extraction and Report Language                                                                                                                                                             
                                ], minima 
                                l         
perl                 5.26 [d]   common [d Practical Extraction and Report Language                                                                                                                                                             
                                ], minima 
                                l         
perl                 5.30       common [d Practical Extraction and Report Language                                                                                                                                                             
                                ], minima 
                                l         
perl                 5.32       common [d Practical Extraction and Report Language                                                                                                                                                             
                                ], minima 
                                l   

and corresponding modulemd_defaults unit in production rhsm-pulp data:
(Maybe the testing code reads wrongly the output as in terminal the formatting doesn't look very nice)

...
            "name": "perl",
            "profiles": {
                "5.24": [
                    "common"
                ],
                "5.26": [
                    "common"
                ],
                "5.30": [
                    "common"
                ],
                "5.32": [
                    "common"
                ]
            },
            "pulp_user_metadata": {},
            "repo_id": "ubi-8-for-x86_64-appstream-rpms__8",
            "stream": "5.26"
        },
...

which say 2 things:

  • what profile is default for which stream
  • what stream is default for the module

so it should be dependent on the data in pulp only, can you double-check the removal and copy of modulemd_defaults units? There might some subtle bug (or the formatting problem as I mentioned above).

@negillett negillett marked this pull request as draft March 28, 2024 19:35
@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

run tests

@negillett negillett marked this pull request as ready for review March 28, 2024 22:10
This commit removes the library's native _pulp_client ("legacy") and
replaces it with usage of pubtools-pulplib methods for associating and
unassociating content.

Additionally, PulpActions were replaced with simple abstractions for
grouping content to be associated/unassociated.

Tests were removed/edited as necessary.
@negillett
Copy link
Member Author

"common [d]" is showing up now for that one tests 🤷🏻

@negillett
Copy link
Member Author

run tests

@negillett
Copy link
Member Author

@rohanpm @rbikar ptal

@rbikar rbikar merged commit 5b57dc6 into release-engineering:master Apr 5, 2024
8 checks passed
@negillett negillett deleted the 141-b branch April 5, 2024 14:42
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.

2 participants