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

[Docs] Logging Configuration - Use logrotate Instead #9930

Merged

Conversation

bagasme
Copy link
Contributor

@bagasme bagasme commented Jan 22, 2020

As @techknowlogick suggest on #9866, I added notes to Logging Configurations if logrotate is about to be used instead of built-in implementation.

When this PR is merged, it will be automatically close #9866.

@bagasme bagasme changed the title Logging Configuration - Use logrotate Instead [Docs] Logging Configuration - Use logrotate Instead Jan 22, 2020
@codecov-io
Copy link

codecov-io commented Jan 22, 2020

Codecov Report

Merging #9930 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9930   +/-   ##
=======================================
  Coverage   42.26%   42.26%           
=======================================
  Files         608      608           
  Lines       79411    79411           
=======================================
  Hits        33566    33566           
  Misses      41702    41702           
  Partials     4143     4143

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 871444f...9eee9d1. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 22, 2020
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 22, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 22, 2020
Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

Wording nits

docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
@techknowlogick techknowlogick added type/docs This PR mainly updates/creates documentation status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels Jan 22, 2020
@techknowlogick techknowlogick removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jan 23, 2020
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

AFAIU this technique could still cause loss of logs.

If we genuinely want to support logrotate we will need some way of halting log output and then reopening it. That would need probably need tying into graceful as a currently unused signal (SIGUSR1 is free but there might be a better one) or by adding another command and private endpoint.

The only safe other options other than using our logging controls, are to use rotatelog from Apache by writing to a pipe or write logs to syslog through systemd

@bagasme
Copy link
Contributor Author

bagasme commented Jan 24, 2020

@zeripath perhaps blocking this PR until halting log output implementation done?

@zeripath
Copy link
Contributor

Yup that's what my request changes does. Unless others disagree and want this merged or the wording is changed to add a sufficient warning - it can't be merged.

@zeripath zeripath added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jan 25, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@633f52c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9930   +/-   ##
=========================================
  Coverage          ?   42.26%           
=========================================
  Files             ?      608           
  Lines             ?    79411           
  Branches          ?        0           
=========================================
  Hits              ?    33561           
  Misses            ?    41709           
  Partials          ?     4141           
Impacted Files Coverage Δ
modules/repofiles/file.go 90.62% <0.00%> (ø)
modules/setting/log.go 69.36% <0.00%> (ø)
models/notification.go 64.13% <0.00%> (ø)
modules/util/sanitize.go 0.00% <0.00%> (ø)
routers/repo/view.go 38.59% <0.00%> (ø)
routers/api/v1/org/team.go 39.82% <0.00%> (ø)
modules/markup/markup.go 77.14% <0.00%> (ø)
modules/structs/user_app.go 0.00% <0.00%> (ø)
models/migrations/v37.go 0.00% <0.00%> (ø)
modules/log/event.go 64.61% <0.00%> (ø)
... and 598 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 633f52c...afb7aef. Read the comment docs.

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Now #11777 has been merged we can finally move forward with this. I expect that the below wording is not the best but it is an example of the kind of thing we need to add.

docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

zeripath commented Jul 6, 2020

Hi @bagasme!

Looks like #11777 has been merged so we might finally be able to get this merged - see my suggestion.

@zeripath zeripath removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jul 13, 2020
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

OK now we have merged my PR causing release and reopen, and updated this - I think we're good to go.

@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit 07c4ed4 into go-gitea:master Jul 16, 2020
@techknowlogick techknowlogick added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jul 16, 2020
@techknowlogick techknowlogick added this to the 1.13.0 milestone Jul 16, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants