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

Add power off/on events to automate control and the foreign key to events physical server #15138

Merged
merged 2 commits into from
Jun 13, 2017

Conversation

AndreyMenezes
Copy link
Member

This PR make the following changes:

  • Add power off/on events to automate control
  • Add foreign key to physical server in miq_events

#
# Physical Servers Operations
#
phs_poweroff,PHS Power OFF,Default,phs_operations
Copy link
Member

Choose a reason for hiding this comment

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

@blomquisg @juliancheal I think it makes sense to separate physical and virtual power operations. We prefix the vm operations with vm. Is there a commonly used prefix you would suggest here?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

phs seems good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion the options are ph, phs or ps.

@@ -26,6 +26,7 @@ class EventStream < ApplicationRecord
belongs_to :container_node

belongs_to :middleware_server, :foreign_key => :middleware_server_id
belongs_to :physical_server, :class_name => "PhysicalServer", :foreign_key => :physical_server_id
Copy link
Member

Choose a reason for hiding this comment

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

It seems not necessary to specify :class_name and :foreign_key.

@AndreyMenezes AndreyMenezes changed the title [WIP] Add power off/on events to automate control and the foreign key to events physical server Add power off/on events to automate control and the foreign key to events physical server May 31, 2017
@miq-bot miq-bot removed the wip label May 31, 2017
@rodneyhbrown7
Copy link

@gmcculloug is this PR good to merge?

#
phs_shutdown,Physical Server Shutdown,Default,phs_operations
phs_start,Physical Server Start,Default,phs_operations
phs_reset,Physical Server Reset,Default,phs_operations
Copy link
Member

Choose a reason for hiding this comment

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

Looking through this file again I have to ask why we are abbreviating these at all? The existing pattern in this file would lead me to say the event names should be: physical_server_*

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you @gmcculloug, I use phs because of the suggestions. But this acronym may confuse new contributors for not being widespread or used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rodneyhbrown7 @juliancheal @lfu @gmcculloug @blomquisg I will change the events names ok? All agree?

Choose a reason for hiding this comment

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

I agree, this is fine.

@AndreyMenezes
Copy link
Member Author

Done

@@ -210,3 +210,10 @@ containerreplicator_successfulcreate,Replicator Successfully Created Pod,Default
containerreplicator_compliance_check,Replicator Compliance Check,Default,compliance
containerreplicator_compliance_passed,Replicator Compliance Passed,Default,compliance
containerreplicator_compliance_failed,Replicator Compliance Failed,Default,compliance

#
# Physical Servers Operations
Copy link
Member

Choose a reason for hiding this comment

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

Should be # Physical Server Operations

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I didn't see this. Done

@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2017

Checked commits AndreyMenezes/manageiq@2b04d09~...25142c9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@AndreyMenezes
Copy link
Member Author

@gmcculloug @lfu It's ok now?

@gmcculloug gmcculloug merged commit 7b50f75 into ManageIQ:master Jun 13, 2017
@chessbyte chessbyte added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 13, 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.

7 participants