Adds a check to only fail policy update if unsafe action is changed#32002
Adds a check to only fail policy update if unsafe action is changed#32002colings86 merged 1 commit intoelastic:index-lifecyclefrom colings86:ilm/updateDiffsActions
Conversation
|
Pinging @elastic/es-core-infra |
| if (newAction == null) { | ||
| return true; | ||
| } else { | ||
| return currentAction.equals(newAction) == false; |
There was a problem hiding this comment.
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.
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.
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.
should we also add a change here to remove the action from the policy?
There was a problem hiding this comment.
you mean as a separate test? The existing Unsafe tests actually already do this as the new policy we create is empty
No description provided.