-
Notifications
You must be signed in to change notification settings - Fork 120
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 policy checking for the retirement request. #86
Conversation
@lfu Cannot apply the following labels because they are not recognized: control, automate |
@tinaafitz @mkanoor Please review |
# | ||
# Description: This method stops the retirement if the policy prevents | ||
# | ||
event = $evm.root['event_stream'] |
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.
@lfu
Can this method be converted to the new style with namespaces.
e.g. https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/AutomationManagement/AnsibleTower/Operations/Methods.class/__methods__/available_credentials.rb
Also can we add a spec for this.
event = $evm.root['event_stream'] | ||
if event.full_data && event.full_data[:policy][:prevented] | ||
$evm.log("info", "#{event.message}") | ||
exit MIQ_STOP |
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.
@lfu In the Spec we might want to stub the exit method. The spec is tested outside of the Automate Engine
# Description: This method stops the retirement if the policy prevents | ||
# | ||
event = $evm.root['event_stream'] | ||
if event.full_data && event.full_data[:policy][:prevented] |
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.
@lfu we should check if event is nil?
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.
@mkanoor I think it can reach this point in automate only if the event stream object exists.
@lfu Should the request_orchestration_stack_retire policy instance be changed as well? |
@tinaafitz request_orchestration_stack_retire is not defined in db/fixtures/miq_event_definitions.csv which means it is not one of the policy events.
Do you think it should be a policy event so users can make policies based on it? If that is the case, it needs to be added into the files first.
|
@lfu have you thought of making this as a builtin method like these |
@lfu we should also be creating a notification when this method stops the process, e.g if retirement is requested and we dont raise a notification users would think retirement is stuck. |
@mkanoor @lfu Yes, it would be important to create a notification if the prevent policy stops the process. Since the method is generic, we could just use "event.message" as the notification message. |
cc89bd9
to
f814bd8
Compare
Checked commit lfu@f814bd8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@miq-bot add_label fine/yes |
@lfu Looks good. |
Add policy checking for the retirement request. (cherry picked from commit c6c51d4)
Fine backport details:
|
Euwe backport (to manageiq repo) details:
|
All events are sent to automate for processing since Event Switchboard PR.
Since then the policy checking for prevent action has changed from a sync process to an async process.
Part of #14641.
https://bugzilla.redhat.com/show_bug.cgi?id=1439331
@miq-bot assign @gmcculloug
@miq-bot add_label bug, darga/yes, euwe/yes