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.

Sign up for GitHub

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

Jump to bottom

Fix Logic, Improve Test Coverage, and Resilience #37

Merged
merged 8 commits into from
Apr 22, 2020
Merged

Fix Logic, Improve Test Coverage, and Resilience #37

merged 8 commits into from
Apr 22, 2020

Conversation

johannes-gehrs
Copy link
Contributor

  • When using an incorrect date pattern for the index format, the comparison of date stings meant that incorrect results could occur. This has lead to unintended deletion of data in our environment. This PR fixes this by comparing Python Date objects instead.
  • It looks to me like the use of <= in the comparison meant that one more day than intended was deleted. See code.
  • The decision logic is factored out into a pure function which is easily testable.
  • Some unit tests have been added.
  • In order to be able to import the module from the unit test file, I needed to rename the file. Module names with dashes cannot be easily imported.

Johannes Gehrs added 5 commits December 17, 2019 11:40
Refactor Cleanup for Testability
dd38483
Add Unit Tests
19f6685
Replace Dash by Underscore
6b0976c 
Using dashes in Python module names is bad.
Specifically it is a problem as we want to import from the unit tests.
Fixes and Cleanup
e407ef0
Polish
ec85928
@giuliocalzolari giuliocalzolari self-requested a review December 18, 2019 09:02
@giuliocalzolari giuliocalzolari self-assigned this Dec 18, 2019
@giuliocalzolari
Copy link
Contributor

Hi @johannes-gehrs thanks for your PR.
can I ask you why do you use a local function https://github.com/cloudreach/aws-lambda-es-cleanup/pull/37/files#diff-aedd2d570ffd28f6b0994ddd7fd88cddR155?

the method that you implement looks great and also allow to implement the unittest, but could we create a new method of the main class ES_Cleanup ?
is there any specific reason why you use a local function should_delete?

@johannes-gehrs
Copy link
Contributor Author

johannes-gehrs commented Dec 18, 2019 •
edited
Loading

If it is a method then you always need to mock "god object" ES_Cleanup in unit tests.

It is doable and if you insist I could refactor to that but I personally like normal functions, especially when we generally think about "pure logic" like we do here. 😄

@johannes-gehrs
Copy link
Contributor Author

If you are uncomfortable with the higher order function (not sure if that is the case) it would be relatively easy to refactor this to use a normal Decider class which instances get initialized with the values we now pass to delete_decider. And then we can call a method decide on the class instance.

@giuliocalzolari
Copy link
Contributor

having a class Decider would be awesome!

@giuliocalzolari
Copy link
Contributor

@johannes-gehrs did you have some time to create the Decider class?

@johannes-gehrs
Copy link
Contributor Author

Sorry for the long delay. I have refactored to a class. Everything else should have stayed the same but do you have a way to test before merging and releasing?

@giuliocalzolari
Copy link
Contributor

Yes I have a script to deploy aws es and create multiple indexes to test it. Right now I deploy only for big changes.

@johannes-gehrs
Copy link
Contributor Author

Let's get this merged?

@giuliocalzolari
Copy link
Contributor

@johannes-gehrs I found some minor issue in the code, I've opened a PR on your repo
can you have a look?
johannes-gehrs#1

@giuliocalzolari giuliocalzolari mentioned this pull request Mar 9, 2020
Open
@giuliocalzolari
Copy link
Contributor

@johannes-gehrs did you have a chance to review it?

@giuliocalzolari
Copy link
Contributor

@johannes-gehrs

giuliocalzolari
giuliocalzolari suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@giuliocalzolari giuliocalzolari left a comment

Choose a reason for hiding this comment

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

can you please merge my minor changes
johannes-gehrs#1

giuliocalzolari
giuliocalzolari suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@giuliocalzolari giuliocalzolari left a comment

Choose a reason for hiding this comment

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

can you please merge my minor changes
johannes-gehrs#1

giuliocalzolari
giuliocalzolari suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@giuliocalzolari giuliocalzolari left a comment

Choose a reason for hiding this comment

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

can you please merge my minor changes
johannes-gehrs#1

giuliocalzolari
giuliocalzolari suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@giuliocalzolari giuliocalzolari left a comment

Choose a reason for hiding this comment

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

I've done some minor change
here johannes-gehrs#1
can you merge you your branch?

@giuliocalzolari giuliocalzolari merged commit a298b34 into cloudreach:master Apr 22, 2020
@giuliocalzolari giuliocalzolari mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giuliocalzolari giuliocalzolari giuliocalzolari approved these changes

Assignees

@giuliocalzolari giuliocalzolari

Labels
enhancement in review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johannes-gehrs @giuliocalzolari