-
Notifications
You must be signed in to change notification settings - Fork 897
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
Rename WebSocketWorker to RemoteConsoleWorker #18337
Conversation
458d64e
to
e782c83
Compare
The links are duplicated in the description |
If we want to rename the role internal name "websocket", I'd think we need to walk settings and convert the roles so customers who have it enabled now will have it enabled after migrating. Something like this |
@@ -17,5 +17,5 @@ scheduler,Scheduler,1,false,region | |||
smartproxy,SmartProxy,0,false,zone | |||
smartstate,SmartState Analysis,0,false,zone | |||
user_interface,User Interface,0,false,region | |||
websocket,Websocket,0,false,region |
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.
This fixture gets seeded at boot into the server_roles table. I don't know if we'd have to drop the existing websocket row from that table.
It might be fine without dropping the role. I don't know if we present the existing server roles rows in the UI.
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.
Well, as we did the migrations, I think this can just go.
@@ -1288,7 +1288,7 @@ | |||
:connection_pool_size: 8 | |||
:memory_threshold: 1.gigabytes | |||
:nice_delta: 1 | |||
:websocket_worker: | |||
:remote_console_worker: |
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.
I'd think we need a migration to walk their settings changes to see if they have any changes to the old websocket_worker role and change that change to the new name. Similar as below for the role name.
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.
Done!
@jrafanie thanks, I'm working on these migrations as I'm trying to start up an appliance with the changes 😅 |
e782c83
to
e518b13
Compare
Checked commit skateman@e518b13 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/miq_server/role_management.rb
lib/vmdb/loggers.rb
spec/models/miq_remote_console_worker/runner_spec.rb
|
Migrations are done, I asked @simaishi to make a build which I've tested. Also took the most recent nightly build (8th of January), did some changes in the settings and ran the migrations...everything works as it should. So this is now ready for review with the rest. |
🎉 |
Since the
WebSocketWorker
will not support only WebSockets, but other (purr, cockpit) protocols as well and it no longer contains ActionCable it is more meaningful to name itRemoteConsoleWorker
as it serves remote consoles. I also changed thewebsocket
role toremote_console
.Parent issue: #18300
UI part: ManageIQ/manageiq-ui-classic#5136
Schema: ManageIQ/manageiq-schema#319
@miq-bot add_label hammer/no
@miq-bot add_reviewer @kbrock
@miq-bot add_reviewer @jrafanie
@miq-bot add_reviewer @djberg96
@miq-bot add_reviewer @Fryguy