-
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
Middleware server group power ops #13741
Conversation
9707fc6
to
00c6d6e
Compare
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.
Hi @Jiri-Kremser
Just a couple of very minor questions, otherwise looks great.
def shutdown_middleware_server(ems_ref, params) | ||
# server group ops | ||
def start_middleware_server_group(ems_ref) | ||
# blocking: boolean |
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.
Hi @Jiri-Kremser - what does this comment mean?
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.
These are the optional parameters the operation can take, I can remove that if it's confusing. Or comment better.
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.
If you wanted an optional param wouldn't you need
def start_middleware_server_group(ems_ref, blocking = false)
or something like that?
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.
blocking
is an optional param in the underlying Wildfly dmr api, but we never set it to false
, so I removed the comment at all and didn't complicate the method signature with it.
|
||
def stop_middleware_server_group(ems_ref, params) | ||
# blocking: boolean | ||
# timeout: int |
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.
@Jiri-Kremser - I'm not sure about the timeout comment either...
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.
So is params
a hash that is expected to have {:blocking, :timeout}
as possible keys?
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.
correct. Possible keys that we are not using now. I am ok with removing the comments or commenting it better.
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.
Oh duh I didn't see this timeout = params[:timeout] || 0
below
I think I'd leave the comments out and if timeout is optional, make params = {}
so it is optional as well
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 removed the comments and made the params hash optional arg. for couple of methods.
# server ops | ||
def shutdown_middleware_server(ems_ref, params) | ||
timeout = params[:timeout] || 0 | ||
run_generic_operation(:Shutdown, ems_ref, :restart => false, :timeout => timeout) |
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.
@Jiri-Kremser - is there a default value for :restart? I see it is false here and true below.
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.
jboss cli says:
./jboss-cli.sh --connect ':read-operation-description(name=shutdown)'
{
"outcome" => "success",
"result" => {
"operation-name" => "shutdown",
"description" => "Shuts down the server via a call to System.exit(0)",
"request-properties" => {
"restart" => {
"type" => BOOLEAN,
"description" => "If true, once shutdown the server will be restarted again",
"expressions-allowed" => false,
"required" => false,
"nillable" => true,
"default" => false
},
"timeout" => {
"type" => INT,
"description" => "The shutdown timeout in seconds. If this is zero (the default) then the server will shutdown immediately. A value larger than zero means the server will wait up to this many seconds for all active requests to finish. A value smaller than zero means that the server will wait indefinitely for all active requests to finish.",
"expressions-allowed" => false,
"required" => false,
"nillable" => true,
"default" => 0
}
},
"reply-properties" => {},
"read-only" => false,
"runtime-only" => true
}
}
So the default is false
, I guess the author of the code wanted to be explicit here. It's not my code btw. I only moved the methods that belongs together closer to each other. Sorry for that, the readability of the diff might be bad.
…s - suspend,resume,restart,stop,start
00c6d6e
to
1233aa2
Compare
Checked commits jkremser/manageiq@fb4df7a~...bfeba58 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
thanks @Jiri-Kremser - looks good. |
LGTM 👍 thanks @Jiri-Kremser ! |
Adding support for server group operations, namely:
@miq-bot add_labels providers/hawkular, enhancement