Skip to content
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

Avoid making checkpoint config Explicitlydisabled when ECS_DATADIR is… #2897

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

angelcar
Copy link
Contributor

@angelcar angelcar commented Jun 15, 2021

Summary

Fixes #2850

There was a bug inparseCheckpoint, which incorrectly set the Checkpoint config as ExplicitlyDisabled when the data directory config was not set. This prevented the Checkpoint config to be overridden by other configuration methods, such as configuration file. Please refer to #2850 for more details

Implementation details

Remove the incorrect assignation of ExplicitlyDisabled

Testing

Tested in personal account that the checkpoint is picked up correctly after this change. Made sure it's backwards compatible.

New tests cover the changes: yes

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@shubham2892 shubham2892 left a comment

Choose a reason for hiding this comment

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

🚢

// if the directory is not set, default to checkpointing off for
// backwards compatibility
if checkPoint.Value == NotSet {
checkPoint.Value = ExplicitlyDisabled
Copy link
Member

@fierlion fierlion Jun 15, 2021

Choose a reason for hiding this comment

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

/// Enabled is a convenience function for when consumers don't care if the value is implicit or explicit
func (b BooleanDefaultFalse) Enabled() bool {
return b.Value == ExplicitlyEnabled
}

there it is. So this ends up true to the original intent when NotSet

@angelcar angelcar merged commit d99e30d into aws:dev Jun 15, 2021
@angelcar angelcar deleted the fix-2850 branch June 15, 2021 22:52
@mythri-garaga mythri-garaga added this to the 1.53.1 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants