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

Rename WebsocketWorker to RemoteConsoleWorker #319

Merged
merged 1 commit into from Jan 14, 2019

Conversation

skateman
Copy link
Member

@skateman skateman commented Jan 8, 2019

Renaming both the role and the worker, dropping old workers from the DB.

Parent issue: ManageIQ/manageiq#18300
Core PR: ManageIQ/manageiq#18337

@miq-bot add_label hammer/no

@jrafanie
Copy link
Member

jrafanie commented Jan 8, 2019

I think there might be other migrations needed...

👍

See: #320

It's fine to just remove the worker rows on upgrade to the version where you renamed the class.

@skateman skateman force-pushed the websocket-to-remote-console branch 3 times, most recently from cec251a to 4221247 Compare January 8, 2019 20:35
@skateman skateman changed the title [WIP] Rename WebsocketWorker to RemoteConsoleWorker Rename WebsocketWorker to RemoteConsoleWorker Jan 9, 2019
@miq-bot miq-bot removed the wip label Jan 9, 2019

say_with_time("Renaming all websocket_worker related SettingsChange keys to remote_console_worker") do
SettingsChange.where("key LIKE '/workers/worker_base/websocket_worker/%'").select('id, key').each do |row|
SettingsChange.where(:id => row.id).update(:key => row.key.sub('websocket_worker', 'remote_console_worker'))
Copy link
Member

Choose a reason for hiding this comment

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

Can't we do this without this second where? We already have the row. Can we remove the .select on line 24 and perform the update on the row?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes 😆 I was thinking too much in SQL 😅


say_with_time("Renaming all websocket server role SettingsChange records to remote_console") do
SettingsChange.where("key = '/server/role' AND value LIKE '%websocket%'").select('id', 'value').each do |row|
SettingsChange.where(:id => row.id).update(:value => row.value.sub('websocket', 'remote_console'))
Copy link
Member

Choose a reason for hiding this comment

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

Same as above


say_with_time("Renaming all remote_console_worker related SettingsChange keys to websocket_worker") do
SettingsChange.where("key LIKE '/workers/worker_base/remote_console_worker/%'").select('id, key').each do |row|
SettingsChange.where(:id => row.id).update(:key => row.key.sub('remote_console_worker', 'websocket_worker'))
Copy link
Member

Choose a reason for hiding this comment

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

same


say_with_time("Renaming all remote_console server role SettingsChange records to websocket") do
SettingsChange.where("key = '/server/role' AND value LIKE '%remote_console%'").select('id', 'value').each do |row|
SettingsChange.where(:id => row.id).update(:value => row.value.sub('remote_console', 'websocket'))
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@jrafanie
Copy link
Member

jrafanie commented Jan 9, 2019

This looks like a great approach! I had a few questions above.

class SettingsChange < ActiveRecord::Base; end

class ServerRole < ActiveRecord::Base
self.inheritance_column = :_type_disabled
Copy link
Contributor

@lpichler lpichler Jan 10, 2019

Choose a reason for hiding this comment

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

only if there is type column on the model #4 (comment)

@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2019

Checked commit skateman@971f43d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

This looks good to me. I hope we got them all. 🤞

@skateman
Copy link
Member Author

Okay, who is brave enough to merge this with its core counterpart as well? @Fryguy @agrare @bdunne @carbonin ?

@bdunne bdunne merged commit aef878f into ManageIQ:master Jan 14, 2019
@bdunne bdunne self-assigned this Jan 14, 2019
@bdunne bdunne added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 14, 2019
@bdunne bdunne added the data label Jan 14, 2019
@skateman skateman deleted the websocket-to-remote-console branch January 14, 2019 13:03
@jrafanie
Copy link
Member

🎉 🏃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants