-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Adds a check to only fail policy update if unsafe action is changed #32002
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
Adds a check to only fail policy update if unsafe action is changed #32002
Conversation
Pinging @elastic/es-core-infra |
if (newAction == null) { | ||
return true; | ||
} else { | ||
return currentAction.equals(newAction) == false; |
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.
since getActionFromPolicy
can, theoretically, return null
, would it be safer to switch these around to
newAction.equals(currentAction) == false
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.
right but if currentAction
is null
then we have a bug because we should never be in a situation where the current step doesn't exist in the current policy. If this does happen I would rather not silently fail returning a value. If this does not seem very friendly to you I can add a null check for currentAction
which throws and IllegalStateException
instead?
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 suppose that would make things super clear, but you're right, it would be a bug... so tossing a stacktrace vs. an illegalstateexception is almost the same thing 😄. all good
StepKey currentStep = new StepKey(randomAlphaOfLength(10), MockAction.NAME, randomAlphaOfLength(10)); | ||
LifecyclePolicy oldPolicy = createPolicy(oldPolicyName, null, currentStep); | ||
|
||
// change the current action so its not equal to the old one by adding a step |
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.
should we also add a change here to remove the action from the policy?
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.
you mean as a separate test? The existing Unsafe tests actually already do this as the new policy we create is empty
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, I missed this. thanks
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.
LGTM
No description provided.