Refs #39078 - Remove host compliance and other subscription accounting bits#11663
Refs #39078 - Remove host compliance and other subscription accounting bits#11663jeremylenz merged 2 commits intoKatello:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the katello_pool factory,
subscription { association(:katello_subscription, organization: @instance.organization) }relies on@instance, which isn't a documented FactoryBot pattern and may not behave as expected; consider usingassociation(:katello_subscription, organization: organization)or anafter(:build)hook instead. - The new
getEntitlementsDisplayValuenow bases the display solely onquantityandcollapsible; if there are cases where an attached-but-non-upstream pool should still be treated as non-editable or non-'Unlimited', it might be worth revisiting the logic that previously usedavailableandupstream_pool_idto ensure UI semantics remain correct.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the katello_pool factory, `subscription { association(:katello_subscription, organization: @instance.organization) }` relies on `@instance`, which isn't a documented FactoryBot pattern and may not behave as expected; consider using `association(:katello_subscription, organization: organization)` or an `after(:build)` hook instead.
- The new `getEntitlementsDisplayValue` now bases the display solely on `quantity` and `collapsible`; if there are cases where an attached-but-non-upstream pool should still be treated as non-editable or non-'Unlimited', it might be worth revisiting the logic that previously used `available` and `upstream_pool_id` to ensure UI semantics remain correct.
## Individual Comments
### Comment 1
<location path="db/migrate/20260225015710_remove_katello_compliance_reasons.katello.rb" line_range="8-17" />
<code_context>
+ remove_column :katello_pools, :virtual
+ end
+
+ def down
+ create_table :katello_compliance_reasons do |t|
+ t.string :reason
+ t.references :subscription_facet
+ end
+
+ add_foreign_key "katello_compliance_reasons", "katello_subscription_facets",
+ :name => "katello_compliance_reasons_facet_id", :column => "subscription_facet_id"
+
+ add_column :katello_pools, :consumed, :integer
+ add_column :katello_pools, :virtual, :boolean
+ end
+end
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The down migration may not fully restore the original schema, which can complicate rollbacks.
In `down`, the recreated `katello_compliance_reasons` table only has `reason` and `subscription_facet` and omits any original indexes, null constraints, or timestamps. The restored `consumed` and `virtual` columns also lack their original defaults/constraints. If production rollbacks are expected, consider matching the original schema definitions, or explicitly raise in `down` to mark the migration as non-reversible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -48,6 +47,10 @@ class Pool < Katello::Model | |||
|
|
|||
| validate :subscription_matches_organization | |||
|
|
|||
| delegate :instance_multiplier, :support_level, :name, :sockets, :cores, to: :subscription | |||
| delegate :cp_id, to: :subscription, prefix: true | |||
| alias_method :product_id, :subscription_cp_id | |||
There was a problem hiding this comment.
These few lines are an interesting change. All of this data is being imported into Katello::Subscription, but was not being referenced when called through a pool object, instead relying on lazy_accessor (now removed). The result is that there are fewer API calls to candlepin when using subscriptions index and show APIs because those pieces now come from the DB.
| trait :inactive do | ||
| active { false } | ||
| end | ||
| subscription { association(:katello_subscription, organization: @instance.organization) } |
There was a problem hiding this comment.
This is the proper way to pass down attributes to associations rather than using after build callbacks
| @@ -38,7 +38,7 @@ export const createSubscriptionsTableSchema = ( | |||
| formatters: [ | |||
| (value, additionalData) => { | |||
| // eslint-disable-next-line no-param-reassign | |||
| additionalData.disabled = !hasPermission || additionalData.rowData.available === -1; | |||
There was a problem hiding this comment.
The gist of the UI changes with respect to available is that (from what I can tell) the presence of available was being used to denote a Red Hat Subscription, at least in most uses. Anyway, available is totally meaningless as consumption is no longer tracked.
This is written so convincingly that I need to think about it some more. TBD... Update: I think it's fine, but some review here would be good to double check me. |
c1fcd7d to
795327a
Compare
|
No further changes planned here pending review |
| def up | ||
| drop_table :katello_compliance_reasons | ||
| remove_column :katello_pools, :consumed | ||
| remove_column :katello_pools, :virtual |
There was a problem hiding this comment.
Just for my own reassurance - My "Requires Virt-who" column still seems to work fine. I guess that's because we still have it on subscription and don't need it on pool?
With SCA-Aware Manifests the plan is to "Remove columns Type, Requires Virt-Who, Consumed and Entitlements from the page" anyway, so I suppose this fulfills that partially!
There was a problem hiding this comment.
Yeah, virtual is totally unused so went for the removal. The virt-who column is driven by the virt_who attribute
795327a to
b816619
Compare
|
Rebased with 1 additional commit that adds back some lazy_accessors I'll deal with in #11647 |
What could go wrong? xD |

What are the changes introduced in this pull request?
Founds some more pieces in relation to #11616
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Most of this can be validated by loading the subscriptions + subscriptions details pages + adding/removing/modifying upstream subscriptions. Ultimately, ensure nothing is calling deleted methods, etc.
Summary by Sourcery
Remove legacy host subscription compliance and subscription consumption accounting, simplifying pool and subscription models, APIs, and UI to rely on core subscription attributes only.
Enhancements:
Deployment:
Tests:
Chores: