Fixes #34211 - Hosts - Change content source #9873

stejskalleos · 2022-01-03T13:42:21Z

What are the changes introduced in this pull request?

New feature for changing host's content source & generating bash code for configuration on the host machines

Considerations taken when implementing this change?

This PR is follow up to [WIP] Job template for content source change #9756

What are the testing steps for this pull request?

UI:

Go to Hosts > All hosts, select hosts and choose Change Content Source option in menu
Change the CS
Run job invocation (or do it manually)
Verify that subscription-manager have been configured properly

API:

Call the API endpoint from host machine and run bash code on it

curl -X 'PUT 'http://katello.example.com/api/hosts/change_content_source' \
  --user admin:changeme \
  -H 'Content-Type: application/json' \
  --data '{"environment_id":"0","content_view_id":"0","content_source_id":"0","host_ids":[0]}' | bash

TODO

React tests

theforeman-bot · 2022-01-03T13:42:23Z

Issues: #34211

evgeni · 2022-01-04T10:06:34Z

[test katello]

jturel

I've left a lot of feedback here and so I haven't tested it end-to-end yet but overall it looks good.

This really needs some test coverage for the larger pieces (API, primary react components).

jturel · 2022-01-04T19:54:52Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+          end
+
+          @content_sources = SmartProxy.authorized(:view_smart_proxies).with_content.includes([:smart_proxy_features])
+          @environments = KTEnvironment.readable.includes([:organization, :env_priors, :priors]).order(:name)


It looks like this form implies changing organization by being able to select an environment that doesn't belong to the host's current organization, and that's made more complicated if you selected hosts that span organizations. Seems like we'd need to guard against this. What do you think?

Hmm, yeah, there should be some checks. I'll look into it

I added WithOrganization so now user must select organization

app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb

jturel · 2022-01-04T19:57:48Z

app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb

+        param :content_view_id, :number, required: true, desc: N_("The id of the content view")
+        param :content_source_id, :number, required: true, desc: N_("The id of the content source")
+        def change_content_source
+          hosts = ::Host.where(id: params[:host_ids])


Do we have an "editable" scope or something similar for Host?

There is a check for edit_hosts permission when calling the endpoint:

def action_permission case params[:action] when 'host_collections' 'edit' when 'change_content_source' 'edit' else super end end

app/helpers/katello/content_source_helper.rb

app/views/foreman/hosts/change_content_source.erb

webpack/index.js

webpack/scenes/ChangeContentSource/actions.js

webpack/scenes/ChangeContentSource/components/cs_form.js

stejskalleos · 2022-01-10T14:50:51Z

@jturel updated, now requires selected organization prior to the change of content source.
@jeremylenz could please do the review of the .js code?

jeremylenz

Thanks @stejskalleos

JS code looks mostly fine; left some comments below. (please note this is code review only and I haven't tested functionality.)

webpack/scenes/Hosts/ChangeContentSource/ChangeContentSourcePage.js

webpack/scenes/Hosts/ChangeContentSource/components/ContentSourceTemplate.js

webpack/scenes/Hosts/ChangeContentSource/helpers.js

webpack/scenes/Hosts/ChangeContentSource/index.js

jturel · 2022-01-14T15:58:26Z

Just started testing but I seem to be getting stuck (button is disabled)

stejskalleos · 2022-01-18T09:27:35Z

Just started testing but I seem to be getting stuck (button is disabled)

Oh sorry my bad, did quick refactoring of formIsValid() function and didn't test it properly. Now it should work + rebased.

jeremylenz

Thanks for doing the JSX refactoring I suggested! Found a couple more places that still need to be updated :)

webpack/scenes/Hosts/ChangeContentSource/actions.js

webpack/scenes/Hosts/ChangeContentSource/components/ContentSourceForm.js

webpack/scenes/Hosts/ChangeContentSource/index.js

jturel · 2022-01-24T15:25:42Z

@jeremylenz has all of your feedback been addressed? If so I can start testing again

jeremylenz

@jturel JS changes look good, thanks @stejskalleos! Left one more wording comment below, but please feel free to commence testing!

webpack/scenes/Hosts/ChangeContentSource/index.js

jturel · 2022-01-24T16:39:42Z

@stejskalleos this looks and works great! nice work.

There's one thing I wanna bring to your attention:

Select my host and go into Change Content Source
Change the organization from the org switcher at the top
I can see and set all the fields but I can't apply the change - which is good

It may be better to not show the any of the selection fields in this scenario and instead show some kind of message about the host not belonging to the selected organization. What do you think?

jeremylenz · 2022-01-24T17:47:38Z

It may be better to not show the any of the selection fields in this scenario and instead show some kind of message about the host not belonging to the selected organization

You can use webpack/components/WithOrganization/withOrganization.js for this

jturel · 2022-01-24T17:57:48Z

It may be better to not show the any of the selection fields in this scenario and instead show some kind of message about the host not belonging to the selected organization

You can use webpack/components/WithOrganization/withOrganization.js for this

I thought withOrganization enforces that an organization is selected rather than ensuring that the selected organization contains the current resource on the page. Or maybe you meant that as a first step in shoring up the issue?

jeremylenz · 2022-01-24T18:12:39Z

I thought withOrganization enforces that an organization is selected rather then ensuring that the selected organization contains the current resource on the page.

Ah, you're right. Been a while. Delete that comment 😄

stejskalleos · 2022-01-25T10:46:33Z

@jeremylenz @jturel fixed the wording and disabled fields when there are no hosts to configure (that case with changing the organization)

jturel

ACK

@jeremylenz I'll merge once this has your approval as well

Thanks @stejskalleos !

jeremylenz

ACK 👍

theforeman-bot added Needs testing Not yet reviewed labels Jan 3, 2022

stejskalleos mentioned this pull request Jan 3, 2022

[WIP] Job template for content source change #9756

Closed

jturel self-assigned this Jan 4, 2022

jturel suggested changes Jan 4, 2022

View reviewed changes

stejskalleos marked this pull request as draft January 7, 2022 13:10

stejskalleos force-pushed the ls/csui branch from c076978 to a07ffc0 Compare January 10, 2022 14:48

stejskalleos marked this pull request as ready for review January 10, 2022 14:49

jeremylenz reviewed Jan 10, 2022

View reviewed changes

stejskalleos force-pushed the ls/csui branch 4 times, most recently from 888c0f5 to cb5dbf3 Compare January 14, 2022 13:04

stejskalleos force-pushed the ls/csui branch from cb5dbf3 to b26d827 Compare January 18, 2022 09:26

stejskalleos force-pushed the ls/csui branch from b26d827 to c9aadcc Compare January 18, 2022 09:33

jeremylenz reviewed Jan 18, 2022

View reviewed changes

stejskalleos force-pushed the ls/csui branch 2 times, most recently from b366bd3 to bb15a27 Compare January 21, 2022 11:41

jeremylenz reviewed Jan 24, 2022

View reviewed changes

webpack/scenes/Hosts/ChangeContentSource/index.js Outdated Show resolved Hide resolved

Fixes #34211 - Hosts - Change content source

857665f

stejskalleos force-pushed the ls/csui branch from bb15a27 to 857665f Compare January 25, 2022 10:45

jturel approved these changes Jan 25, 2022

View reviewed changes

jeremylenz approved these changes Jan 25, 2022

View reviewed changes

jturel merged commit c5f8e95 into Katello:master Jan 25, 2022

stejskalleos mentioned this pull request May 5, 2022

Fixes #34865 - New host detail - Change content source #10091

Merged

Gauravtalreja1 mentioned this pull request Sep 13, 2022

Update expected API endpoints for 6.12 SatelliteQE/robottelo#10049

Merged

stejskalleos mentioned this pull request Oct 12, 2022

Fixes #35446 - Change content source improvements #10297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #34211 - Hosts - Change content source #9873

Fixes #34211 - Hosts - Change content source #9873

stejskalleos commented Jan 3, 2022 •

edited

theforeman-bot commented Jan 3, 2022

evgeni commented Jan 4, 2022

jturel left a comment

jturel Jan 4, 2022

stejskalleos Jan 5, 2022

stejskalleos Jan 14, 2022

jturel Jan 4, 2022

stejskalleos Jan 5, 2022

stejskalleos commented Jan 10, 2022

jeremylenz left a comment

jturel commented Jan 14, 2022

stejskalleos commented Jan 18, 2022

jeremylenz left a comment

jturel commented Jan 24, 2022

jeremylenz left a comment

jturel commented Jan 24, 2022

jeremylenz commented Jan 24, 2022

jturel commented Jan 24, 2022 •

edited

jeremylenz commented Jan 24, 2022

stejskalleos commented Jan 25, 2022

jturel left a comment

jeremylenz left a comment

Fixes #34211 - Hosts - Change content source #9873

Fixes #34211 - Hosts - Change content source #9873

Conversation

stejskalleos commented Jan 3, 2022 • edited

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

TODO

theforeman-bot commented Jan 3, 2022

evgeni commented Jan 4, 2022

jturel left a comment

Choose a reason for hiding this comment

jturel Jan 4, 2022

Choose a reason for hiding this comment

stejskalleos Jan 5, 2022

Choose a reason for hiding this comment

stejskalleos Jan 14, 2022

Choose a reason for hiding this comment

jturel Jan 4, 2022

Choose a reason for hiding this comment

stejskalleos Jan 5, 2022

Choose a reason for hiding this comment

stejskalleos commented Jan 10, 2022

jeremylenz left a comment

Choose a reason for hiding this comment

jturel commented Jan 14, 2022

stejskalleos commented Jan 18, 2022

jeremylenz left a comment

Choose a reason for hiding this comment

jturel commented Jan 24, 2022

jeremylenz left a comment

Choose a reason for hiding this comment

jturel commented Jan 24, 2022

jeremylenz commented Jan 24, 2022

jturel commented Jan 24, 2022 • edited

jeremylenz commented Jan 24, 2022

stejskalleos commented Jan 25, 2022

jturel left a comment

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

stejskalleos commented Jan 3, 2022 •

edited

jturel commented Jan 24, 2022 •

edited