Skip to content

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

Merged
merged 1 commit into from
Jul 12, 2018
Merged

Adds a check to only fail policy update if unsafe action is changed #32002

merged 1 commit into from
Jul 12, 2018

Conversation

colings86
Copy link
Contributor

No description provided.

@colings86 colings86 added review :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Jul 12, 2018
@colings86 colings86 self-assigned this Jul 12, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

if (newAction == null) {
return true;
} else {
return currentAction.equals(newAction) == false;
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@colings86 colings86 merged commit ebf0dd4 into elastic:index-lifecycle Jul 12, 2018
@colings86 colings86 deleted the ilm/updateDiffsActions branch July 12, 2018 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants