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

Load ProductID certificate metadata via pushsource [RHELDST-24276] #284

Conversation

MichalHaluza
Copy link
Contributor

@MichalHaluza MichalHaluza commented May 7, 2024

python-rhsm cannot be installed with GCC 14. Given that project has been deprecated for years, it's unlikely the actual underlying issue will be resolved. Pubtools-pulp uses python-rhsm to read ProductID certificates and pull product metadata from them. This commit leverages code for ProductID reading newly introduced in Pushsource instead of python-rhsm.

This MR also replaces rpm-py-installer with rpmdyn in the same fashion like it was done in release-engineering/pushsource@1c1b4f9

@MichalHaluza
Copy link
Contributor Author

This MR depends on release-engineering/pushsource#476

Due to rpm-py-installer/rpm-py-installer#276, it was necessary to pin a
particular version of virtualenv.

Let's undo that pin and try an alternative provider of RPM bindings
instead. This library works a different way which does not require
downloading and compiling from RPM sources, and is compatible with the
newest versions of pip and other tools.
@MichalHaluza MichalHaluza force-pushed the load_productid_via_pushsource branch 2 times, most recently from 661363b to 317eaf9 Compare May 10, 2024 08:39
@MichalHaluza MichalHaluza changed the title Draft: Load ProductID certificate metadata via pushsource [RHELDST-24276] Load ProductID certificate metadata via pushsource [RHELDST-24276] May 10, 2024
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5166766) to head (04a496e).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #284   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           57        57           
  Lines         3170      3168    -2     
=========================================
- Hits          3170      3168    -2     

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

rbikar
rbikar previously approved these changes May 10, 2024
rohanpm
rohanpm previously approved these changes May 13, 2024
tests/push/test_productid_updates_versions.py Outdated Show resolved Hide resolved
@MichalHaluza MichalHaluza dismissed stale reviews from rohanpm and rbikar via f665846 May 13, 2024 06:52
@MichalHaluza MichalHaluza force-pushed the load_productid_via_pushsource branch 2 times, most recently from f665846 to 317eaf9 Compare May 13, 2024 06:54
rohanpm
rohanpm previously approved these changes May 13, 2024
rohanpm
rohanpm previously approved these changes May 14, 2024
# make a fake productid.
# content doesn't matter since we patched the cert parser, it just has
# content doesn't matter since we mock the ProductIDs, it just has
Copy link
Member

Choose a reason for hiding this comment

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

Err... sorry to be pedantic again about this comment (but you have said in other reviews that you like it).

I don't think "mock" is the right word here, and I think using it contributes to confusion around what mocking is. Well, I don't think it's a very well-defined term, but I agree with Wikipedia's leading sentence: "A mock object is an object that imitates a production object in limited ways." You are providing real ProductId objects here, which are going to work in exactly the same way as ProductId objects loaded out of a cert. You're using the API of pushsource in a normal and valid way and I don't see how it should be considered "mocking". You're just providing your own data which skips the default behavior of trying to load that data from a file.

If I may give an analogy, if there was a function with a signature like:

def load_config(path: str = "/etc/some-default.cfg"):
    ...

...and some code were to call that function like cfg = load_config("my-non-default.cfg"), I don't think that should be called "mocking" the path or the config?

I believe it's better for the comment to say "we provide our own ProductIds", and also for us to think about the code in this way. Or if you wanted to emphasize that this is overriding the default method of obtaining the objects, you could even use the term "inject" per dependency injection, "a programming technique in which an object or function receives other objects or functions that it requires, as opposed to creating them internally".

python-rhsm cannot be installed with GCC 14. Given that project
has been deprecated for years, it's unlikely the actual underlying
issue will be resolved. Pubtools-pulp uses python-rhsm to read
ProductID certificates and pull product metadata from them. This commit
leverages code for ProductID reading newly introduced in Pushsource
instead of python-rhsm.
@MichalHaluza MichalHaluza merged commit 2a17378 into release-engineering:master May 16, 2024
9 checks passed
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.

3 participants