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

Migration: Add health_state and power_state columns to physical server #14042

Merged
merged 2 commits into from
Mar 10, 2017
Merged

Migration: Add health_state and power_state columns to physical server #14042

merged 2 commits into from
Mar 10, 2017

Conversation

rodneyhbrown7
Copy link

@miq-bot add_label: wip, providers/physical-infrastructure

@rodneyhbrown7 rodneyhbrown7 changed the title Migration: Add health_state and power_state columns to physical server [WIP] Migration: Add health_state and power_state columns to physical server Feb 23, 2017
@chessbyte chessbyte requested a review from blomquisg February 23, 2017 14:01
@rodneyhbrown7
Copy link
Author

Closing to re-initiate travis tests.

@rodneyhbrown7
Copy link
Author

Reopen to run travis tests

@rodneyhbrown7 rodneyhbrown7 reopened this Feb 23, 2017
@juliancheal
Copy link
Member

juliancheal commented Mar 2, 2017

Missing addition of new column to schema.yml, y'all just need to add health_state and power_state to the PhysicalServer Entry.

Otherwise LGTM 👍

@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2017

Checked commits https://github.com/lenovo/manageiq/compare/4ff1b4652cd880c7358555516968e5c9ebe44473~...686c805555453db79073eb7328e99252a31c14be with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

@rodneyhbrown7
Copy link
Author

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Migration: Add health_state and power_state columns to physical server Migration: Add health_state and power_state columns to physical server Mar 2, 2017
@miq-bot miq-bot removed the wip label Mar 2, 2017
@chargio
Copy link
Contributor

chargio commented Mar 7, 2017

@juliancheal @blomquisg Could you review this?
We need to have this merged for all the other PR to work

@chargio
Copy link
Contributor

chargio commented Mar 9, 2017

Hi @juliancheal @blomquisg, @Fryguy How do you see this PR? Schema freeze is getting close

@@ -0,0 +1,5 @@
class AddPowerStatePropertyToPhysicalServer < ActiveRecord::Migration[5.0]
def change
add_column :physical_servers, :power_state, :string
Copy link
Member

Choose a reason for hiding this comment

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

My only concern is that in the other tables we have separated the concept of "raw_power_state" and "power state". "raw_power_state" is the raw string that we get from the provider, whereas "power_state" is a normalized value, mainly used for reporting. cc @blomquisg

I'll merge anyway, but you might need a followup PR to add "raw_power_state". Also, please be sure that usage of the power state column is limited to the regular normalized values (like what we show in the quadicons in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

PR to fix this #14287

@Fryguy Fryguy merged commit 834b4a1 into ManageIQ:master Mar 10, 2017
@Fryguy Fryguy added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 10, 2017
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.

6 participants