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

cleanup-log-directory #1354

Merged
merged 1 commit into from
Oct 11, 2019
Merged

cleanup-log-directory #1354

merged 1 commit into from
Oct 11, 2019

Conversation

riya-17
Copy link
Member

@riya-17 riya-17 commented Sep 23, 2019

Fixes #975

This PR works on cleaning the logs directory if there are no errors.

@riya-17 riya-17 requested review from ndokos and portante September 23, 2019 11:57
@riya-17 riya-17 self-assigned this Sep 23, 2019
@portante portante added this to the v0.65 milestone Sep 23, 2019
portante
portante requested changes Sep 23, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Just a few comments below, looks good.

server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
@portante portante modified the milestones: v0.65, v0.66 Sep 24, 2019
@riya-17 riya-17 force-pushed the cleanup-log-dirs branch 5 times, most recently from ad85986 to 84495ae Compare September 25, 2019 11:21
ndokos
ndokos requested changes Sep 27, 2019
View reviewed changes
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

No need for loops: if there are no errors, the script should delete the directory for this particular run only (the one with the current timestamp). It should not touch any other directories or the change_state.log file.

@riya-17
Copy link
Member Author

riya-17 commented Sep 30, 2019

@ndokos Please Take a Look

portante
portante requested changes Sep 30, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, Riya!

Can you dig a bit deeper on this one? Please see the code-review comment in-line.

server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
@riya-17
Copy link
Member Author

riya-17 commented Oct 3, 2019

@ndokos @portante Please take a Look

ndokos
ndokos requested changes Oct 7, 2019
View reviewed changes
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
portante
portante requested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Coming along, Riya!

server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
@riya-17
Copy link
Member Author

riya-17 commented Oct 7, 2019

@ndokos @portante PTAL

@ndokos ndokos self-requested a review October 8, 2019 17:37
ndokos
ndokos requested changes Oct 8, 2019
View reviewed changes
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

Unit tests need to be fixed as well.

server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
portante
portante requested changes Oct 9, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

And can you consider applying the previous comments I had made?

@riya-17 riya-17 force-pushed the cleanup-log-dirs branch 2 times, most recently from 332fb73 to 1051ca3 Compare October 9, 2019 08:04
@riya-17
Copy link
Member Author

riya-17 commented Oct 9, 2019

@ndokos @portante PTAL

@ndokos ndokos self-requested a review October 10, 2019 21:37
portante
portante requested changes Oct 11, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

So close, just one more piece to make sure we don't leave a log file around unnecessarily.

server/bin/pbench-sync-satellite.sh Outdated Show resolved Hide resolved
@ndokos ndokos self-requested a review October 11, 2019 17:38
portante
portante approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Yeah!

@ndokos
Copy link
Member

ndokos commented Oct 11, 2019

@gurbirkalsi: The e2e tests failed. I restarted the build just in case it was a fluke.

@ndokos ndokos merged commit 79b6887 into master Oct 11, 2019
@riya-17 riya-17 deleted the cleanup-log-dirs branch October 14, 2019 06:48
@ndokos ndokos mentioned this pull request Oct 17, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@portante portante portante approved these changes

@ndokos ndokos ndokos approved these changes

Assignees

@riya-17 riya-17

Labels
Background Tasks bug Server
Projects
None yet
Milestone
v0.66
Development

Successfully merging this pull request may close these issues.

Fix sync-satellite to clean up log directories when no errors
3 participants
@riya-17 @ndokos @portante