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

feat: remove content access mode cache #3471

Merged

Conversation

DuckBoss
Copy link
Contributor

This patch aims to remove references to the 'ContentAccessModeCache' and refactor/simplify related code that
checks this cache since entitlement is being phased out and SCA will be the only content access mode.

Resolves: CCT-619

Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It generally LGTM, there are a couple of things to fix.


Regarding the failure of CertSorterTests::test_missing_installed_product: that test is practically useless now.
The reason for this is:

  • in the test setup, StubContentAccessModeCache was set up for CONTENT_ACCESS_MODE_CACHE: since that stub cache had no content, the comparison in is_simple_content_access() against "org_environment" always returned false
  • is_simple_content_access() now stops using CONTENT_ACCESS_MODE_CACHE, so it always returns true now (thus SCA)
  • ComplianceManager (from which CertSorter derives) fills unentitled_products based on the compliance information, which is an entitlements-only feature (see ComplianceManager._parse_server_status(), the code guarded by is_sca in particular)
  • hence CertSorter.unentitled_products is always empty in SCA mode

In case you are wondering: yes, the other tests that check CertSorter.unentitled_products in any way are sort of dead now. To make them working again, ideally is_simple_content_access() should be mocked/patched for them to return false (and thus ensure things work in entitlement mode). OTOH, dropping compliance bits is a potential further step.


Also make sure to remove the old cache file on upgrade: see the existing code in %posttrans that removes an old cache. Simply remove the dropped cache there as well.

@ptoscano
Copy link
Contributor

To make them working again, ideally is_simple_content_access() should be mocked/patched for them to return false (and thus ensure things work in entitlement mode).

Or, even better, do the same approach that CertSorterSCATests does: patch is_simple_content_access() in setUp() to always return false, so all the tests in CertSorterTests work again in entitlement mode.

@DuckBoss DuckBoss force-pushed the jajerome/remove-cont-acc-mode-cache branch 2 times, most recently from c751794 to 83ff5dd Compare November 19, 2024 08:04
@DuckBoss
Copy link
Contributor Author

Thanks for the detailed review!

I used the approach you suggested to patch is_simple_content_access() in the setUp() function of the CertSorterTests class. The other test cases seem to correctly be using the overwritten return value from the setUp() function, but the test_missing_installed_product did not, so I had to patch it explicitly for that test case.

I agree that dropping the compliance bits would be a good next step. I'm leaving that out of this PR to be addressed in a separate card.

@DuckBoss DuckBoss changed the title [WIP] feat: remove content access mode cache feat: remove content access mode cache Nov 19, 2024
@DuckBoss DuckBoss marked this pull request as ready for review November 19, 2024 08:12
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Additional nice followups can be

  • dropping is_simple_content_access() altogether and related code
  • dropping the compliance bits

Waiting for QE pre-verification.

@jstavel
Copy link
Contributor

jstavel commented Nov 26, 2024

as a qe I verify that it works. After I installed packages from the given copr build and updated system the file content_access_mode.json has been removed during register/unregister action.

This patch aims to remove references to the
'ContentAccessModeCache' and refactor/simplify related
code that checks this cache since entitlement is being phased
out and SCA will be the only content access mode.

Resolves: CCT-619

Signed-off-by: Jason Jerome <[email protected]>
@ptoscano ptoscano force-pushed the jajerome/remove-cont-acc-mode-cache branch from 83ff5dd to 3e4cbc0 Compare November 27, 2024 10:30
@ptoscano ptoscano merged commit 011c8c4 into candlepin:main Nov 28, 2024
21 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