-
Notifications
You must be signed in to change notification settings - Fork 305
Fixes #38823 - Remove auto_attach and autoheal #11526
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
Fixes #38823 - Remove auto_attach and autoheal #11526
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/controllers/api/v2/hosts_controller_test.rb:259-266` </location>
<code_context>
Katello::Candlepin::Consumer.any_instance.stubs(:installed_products).returns([])
host = FactoryBot.create(:host, :with_subscription, :with_operatingsystem)
- host.subscription_facet.update!(:autoheal => true,
+ host.subscription_facet.update!(:service_level => 'Premium',
:installed_products_attributes => [{:product_name => 'foo', :version => '6', :product_id => '69'}])
- put :update, params: { :id => host.id, :subscription_facet_attributes => {:autoheal => false} }
+ put :update, params: { :id => host.id, :subscription_facet_attributes => {:service_level => 'Standard'} }
assert_response :success
- refute host.reload.subscription_facet.reload.autoheal
+ assert_equal 'Standard', host.reload.subscription_facet.reload.service_level
assert_equal 'foo', host.subscription_facet.installed_products.first.name
</code_context>
<issue_to_address>
**suggestion (testing):** Test for updating autoheal removed and replaced with service_level.
Also, add tests for invalid and missing service_level values to ensure robust coverage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
277eb54 to
6363148
Compare
6363148 to
0a93afc
Compare
f8aeff8 to
676996d
Compare
676996d to
906c30f
Compare
chris1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested:
Hammer was tested in Katello/hammer-cli-katello#1006
virt-who checkin - Pass
activation-key create - Pass
activation-key update - Pass
Import a manifest - Pass
Refresh a manifest - Pass
Register a host - Pass
Look at host details to make sure fields are not preset - Pass
Check the api response to make sure fields are not present when looking at a host - Pass
Add subscriptions to manifest - Pass
I found some subscription related stuff in the controllers, since you are removing add/remove subscription endpoints, should we remove these?
https://github.com/jeremylenz/katello/blob/906c30f3b7f0ea186b8aac330adac0185c2cb2b9/app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb#L16
https://github.com/jeremylenz/katello/blob/906c30f3b7f0ea186b8aac330adac0185c2cb2b9/app/controllers/katello/api/v2/hosts_bulk_actions_controller.rb#L19
https://github.com/jeremylenz/katello/blob/906c30f3b7f0ea186b8aac330adac0185c2cb2b9/app/controllers/katello/api/v2/host_subscriptions_controller.rb#L37
https://github.com/jeremylenz/katello/blob/906c30f3b7f0ea186b8aac330adac0185c2cb2b9/app/controllers/katello/api/v2/activation_keys_controller.rb#L10
|
@chris1984 Good catches! I think we got it all now, let's see |
chris1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they are all gone, just one in question:
Should we remove this one too or at least change the action from create?
katello/config/routes/overrides.rb
Line 83 in 1f234b5
| match '/subscriptions/' => 'host_subscriptions#create', :via => :post |
Removed add_subscriptions and remove_subscriptions endpoints and related code: - Removed subscription methods from activation_keys and hosts_bulk_actions controllers - Removed subscription routes from API and overrides - Removed subscription permissions from permission files - Removed subscription-related tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
98068c1 to
8dad61c
Compare
Turns out this one should stay. host_subscriptions#create is used in I did find and fix the wording on one of the params (it mentioned "auto-healing") so thanks for catching that! |
Thanks for fixing the wording! |
What are the changes introduced in this pull request?
Now that all organizations are SCA-only, this PR removes obsolete subscription management functionality:
auto_attachattribute from activation keysautohealattribute from subscription facetsConsiderations taken when implementing this change?
From Jeremy:
I know this is a lot of lines / files changed. But if you remove the Dynflow actions, you must also remove any API endpoints and params that use them. So it makes sense to remove them together.
From Claude:
What are the testing steps for this pull request?
Run the migration
Idk.. test activation key create/update and host content access, make sure everything still works
Note: If you switch away from the branch, you'll want to run rails db:rollback