-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fixes #13667: Add condition_once and execute_once generic methods #849
Conversation
Do not merge for now, wip |
e43e02c
to
4e2d6c6
Compare
Commit modified |
4e2d6c6
to
69ae5a6
Compare
Commit modified |
69ae5a6
to
76b3e54
Compare
Commit modified |
76b3e54
to
314647f
Compare
Commit modified |
314647f
to
234dc15
Compare
Commit modified |
|
||
commands: | ||
reserved:: | ||
"${command}" |
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.
why not using command_execution ?
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.
Makes sense indeed (with command_execution_result
)
234dc15
to
e2b6475
Compare
Commit modified |
e2b6475
to
979a997
Compare
Commit modified |
979a997
to
fc816c3
Compare
Commit modified |
fc816c3
to
02b19f7
Compare
Commit modified |
02b19f7
to
a3c92df
Compare
Commit modified |
a3c92df
to
63d5765
Compare
Commit modified |
"phDDD" usebundle => command_execution_once("exit 4", "${ok_codes}", "ok", "testD"); | ||
|
||
"enable" usebundle => set_dry_run_mode("true"); | ||
"phE" usebundle => command_execution_once("exit 5", "${ok_codes}", "ok", "testE"); |
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.
auditing execute_once is an error ? this is surprising, but i don't know what it should be
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.
This is the current behavior with other command methods, I think it's the best we can do.
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'm adding a test for already executed command in audit mode
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.
done
63d5765
to
2b94157
Compare
Commit modified |
2b94157
to
899a3c5
Compare
Commit modified |
899a3c5
to
1838973
Compare
Commit modified |
"failure" usebundle => _classes_failure("${old_class_prefix}"); | ||
"failure" usebundle => _classes_failure("${class_prefix}"); | ||
|
||
pass3.already_set:: |
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.
already_set is set if the flag was already reserved - so if something else reserved it previously, but didn't set it, it will be reported as success. Is it expected ?
1838973
to
6e9d741
Compare
Commit modified |
@@ -54,7 +54,7 @@ bundle agent permissions_recurse(path, mode, owner, group) | |||
"reenable_reporting_${class_prefix}" | |||
usebundle => enable_reporting, | |||
ifvarclass => "should_report"; | |||
"new result classes" usebundle => _classes_copy("${inner_class}", "${class_prefix}"); | |||
"new result classes" usebundle => _classes_copy("${inner_class_prefix}", "${class_prefix}"); |
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.
this should not be part of this PR
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.
definitely, I'll open a ticket (I just saw this while copy/pasting the inner class code)
tree/20_cfe_basics/flag_lib.cf
Outdated
@@ -49,6 +49,8 @@ | |||
# * `ok`: if the flag was correctly reserved | |||
# * `failed`: if the flag could not be reserved. I can happen if it was reserved by another agent. | |||
# | |||
# * `cancel_flag` to remove a flag reservation. This does not affect already set classes. |
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.
the code DO cancel the persistent classes.
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.
YES, good spot, s/classes/flags
@@ -169,16 +198,17 @@ bundle agent set_flag(name) { | |||
"notreserved" | |||
bundle_return_value_index => "1", | |||
comment => "${flag_unique.value}", | |||
if => "!flag_reserved_${cname}"; | |||
if => "!reserved"; |
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.
notreserved is never used - is it for future usage ?
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.
It is actually used for more precise testing (and maybe for future usage too).
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.
Great ! Thank you
# @parameter until Try to execute the command until a particular state: 'ok', 'any' (defaults to 'any') | ||
# @parameter_constraint until "allow_empty_string" : true | ||
# @parameter_constraint until "select" : [ "", "any", "ok" ] | ||
# @parameter unique_id To identify the command (allows to change the command without losing track) |
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.
shouldn't it be optionnal, and default to command ?
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.
would be better indeed!
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.
This is awesome ! just two question, about uniqueid, and the notreserved usage, and one requested change (permissions is changed)
6e9d741
to
72f75f0
Compare
Commit modified |
72f75f0
to
06da08a
Compare
Commit modified |
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/13667