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

log rotation: max archives #5831

Merged
merged 13 commits into from
Jul 19, 2019
Merged

log rotation: max archives #5831

merged 13 commits into from
Jul 19, 2019

Conversation

javicrespo
Copy link
Contributor

@javicrespo javicrespo commented May 12, 2019 •
edited
Loading

Implements #5556.

@javicrespo javicrespo marked this pull request as ready for review May 12, 2019 19:51
@javicrespo javicrespo mentioned this pull request May 12, 2019
Closed
@hanshasselberg hanshasselberg self-assigned this Jul 2, 2019
hanshasselberg
hanshasselberg requested changes Jul 3, 2019
View reviewed changes
Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

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

Thank you for your work! I left some comments but in general it looks good and we would like to merge.

agent/config/runtime_test.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
website/source/docs/agent/options.html.md Show resolved Hide resolved
agent/config/builder.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile_test.go Outdated Show resolved Hide resolved
freddygv
freddygv suggested changes Jul 3, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv 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 the PR! We're looking forward to getting this in the next release.

I just have a few suggestions inline, building off @i0rek's review.

logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile.go Outdated Show resolved Hide resolved
logger/logfile_test.go Outdated Show resolved Hide resolved
logger/logfile_test.go Outdated Show resolved Hide resolved
logger/logfile_test.go Outdated Show resolved Hide resolved
logger/logfile_test.go Outdated Show resolved Hide resolved
@freddygv freddygv assigned freddygv and unassigned hanshasselberg Jul 3, 2019
@freddygv
Copy link
Contributor

Hi @javicrespo let us know if you are still interested in completing this PR.

@javicrespo
Copy link
Contributor Author

Hey @freddygv, I still am. I’ll try to find somr time this week or the next to work on it

javicrespo and others added 11 commits July 19, 2019 15:18
@javicrespo @freddygv
Update logger/logfile.go
a461edb 
Co-Authored-By: Freddy <freddygv@users.noreply.github.com>
@javicrespo @hanshasselberg
Update logger/logfile.go
d27827b 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile.go
9a403e1 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile.go
4a707d6 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile.go
dca8795 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile.go
0df231b 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile.go
94d6c17 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo @hanshasselberg
Update logger/logfile_test.go
56c63d9 
Co-Authored-By: Hans Hasselberg <me@hans.io>
@javicrespo
PR review related changes
21a2a56
@javicrespo
go fmt
b018a1c
@javicrespo
Merge branch 'master' into log-rotation-max-archives
583e313
freddygv
freddygv suggested changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv 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 addressing all our comments @javicrespo!

Just have one last re-wording suggestion for the docs before we merge.

website/source/docs/agent/options.html.md Outdated Show resolved Hide resolved
@javicrespo @freddygv
Update website/source/docs/agent/options.html.md
70b5f7b 
Co-Authored-By: Freddy <freddygv@users.noreply.github.com>
freddygv
freddygv approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Great, thanks again!

@freddygv freddygv merged commit b006060 into hashicorp:master Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanshasselberg hanshasselberg hanshasselberg requested changes

@freddygv freddygv freddygv approved these changes

Assignees

@freddygv freddygv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@javicrespo @freddygv @hanshasselberg