-
Notifications
You must be signed in to change notification settings - Fork 11
User/jf/remove old complete status file #205
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
Conversation
rane-rajasi
left a comment
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.
Comments inline
…into user/jf/remove_old_complete_status_file
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 90.19% 90.23% +0.04%
==========================================
Files 90 90
Lines 14483 14533 +50
==========================================
+ Hits 13063 13114 +51
+ Misses 1420 1419 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…into user/jf/remove_old_complete_status_file
kjohn-msft
left a comment
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.
Please review the comment inline with @rane-rajasi
7578725 to
dbe796b
Compare
rane-rajasi
left a comment
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.
Comment inline
d8efa2f to
d44d724
Compare
6759eec to
10dd4c6
Compare
…into user/jf/remove_old_complete_status_file
…hub.com/Azure/LinuxPatchExtension into user/jf/remove_old_complete_status_file
rane-rajasi
left a comment
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.
Left minor comments
…re 1.complete.status has highest getmtim()
rane-rajasi
left a comment
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.
@feng-j678 I've left one minor comment. Please do take a look at it before merging
rane-rajasi
left a comment
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.
Reviewed from the extension lifecycle perspective as well, doesn't seem to change any existing functionality or adding orphan files during other handler events (i.e. non-enable)
[x] add logic to remove old complete status file in status folder
[x] refactor json.loads(message) in load_status_file_components()
[x] add unit test for remove old complete status file()