Roll all logs by default #1379

Merged
merged 11 commits into from Feb 8, 2017

Projects

None yet

4 participants

@julianvmodesto
Collaborator
julianvmodesto commented Jan 26, 2017 edited

Issue

#1372

TODO

  • rip out nested block parsing
  • add new subdirectives + share between logging middleware
  • set defaults to rotate logs
  • update docs

work in progress!

@julianvmodesto julianvmodesto Use new subdirectives and flatten rolling config
420266c
@CLAassistant
CLAassistant commented Jan 26, 2017 edited

CLA assistant check
All committers have signed the CLA.

julianvmodesto added some commits Jan 26, 2017
@julianvmodesto julianvmodesto Set default rotate config 3501429
@julianvmodesto julianvmodesto Set default rolling config (hopefully) errwhere
c2e899f
@julianvmodesto
Collaborator
julianvmodesto commented Jan 26, 2017 edited

Ok, looks like this implements the desired behavior for default rolling logs config.

# old
log ... {
    rotate {
        size maxsize
        age maxage
        keep maxkeep
    }
}

# new
log ... {
    rotate_size maxsize
    rotate_age maxage
    rotate_keep maxkeep
}

# old
errors {
    log ... {
      size maxsize
      age maxage
      keep maxkeep
    }
}

# new
errors {
    log ... {
      rotate_size maxsize
      rotate_age maxage
      rotate_keep maxkeep
    }
}

Does this PR do enough to rip out the original nested block parsing for log?

The log directive was flattened by removing the rotate subdirective, but I can't seem to find an alternative to c.IncrNest() for parsing both the log and errors directives.

See here.

@julianvmodesto julianvmodesto Make private
b46ad75
@mholt
Owner
mholt commented Jan 27, 2017

Thanks for starting on this!

Let's flatten this:

errors {
    log ... {
      rotate_size maxsize
      rotate_age maxage
      rotate_keep maxkeep
    }
}

down to this:

errors {
      rotate_size maxsize
      rotate_age maxage
      rotate_keep maxkeep
}

We can get rid of the whole IncrNest() function I think, since nesting isn't really a thing without this; it won't be needed.

julianvmodesto added some commits Jan 29, 2017
@julianvmodesto julianvmodesto Flatten errors directive and remove c.IncrNest()
450b22e
@julianvmodesto julianvmodesto Don't skip first error log roller subdirective we see
fff6463
@julianvmodesto julianvmodesto Remove hadBlock
df6df9e
@julianvmodesto
Collaborator

Got it, flattened + removed IncrNest()

@mholt
Owner
mholt commented Jan 30, 2017

Nice work 👍 Thanks. I'm away for a week but will get back to this ASAP!

@mholt
mholt approved these changes Feb 7, 2017 View changes

Great work! :) This looks good. There's a merge conflict... can you fix it, then I'll merge this in?

Thank you for your contribution!!

@julianvmodesto julianvmodesto Merge branch 'master' into rolling-logs-🌲
34aafa2
caddyhttp/httpserver/roller.go
-
- "github.com/mholt/caddy"
-
- "gopkg.in/natefinch/lumberjack.v2"
@mholt
mholt Feb 7, 2017 Owner

Oops, this import needs to be restored

@julianvmodesto
julianvmodesto Feb 7, 2017 Collaborator

Agh, re-imported!

julianvmodesto added some commits Feb 7, 2017
@julianvmodesto julianvmodesto Try lumberjack import
d0d38b2
@julianvmodesto julianvmodesto Unname import
8e602c6
@julianvmodesto
Collaborator

Hm, anything I should do to fix the AppVeyor build?

@mholt
Owner
mholt commented Feb 7, 2017

That's weird. GitHub bug? It definitely passed: https://ci.appveyor.com/project/mholt/caddy/build/2195

I'll merge it in soon. :) I'm at school for a bit.

@mholt
Owner
mholt commented Feb 8, 2017

@julianvmodesto Can you resolve the merge conflicts?

And before I merge, have you verified that there is/are test cases to ensure logs are rolled by default and the default configuration is properly overwritten when specified?

@julianvmodesto julianvmodesto Merge branch 'master' into rolling-logs-🌲
60415c8
@julianvmodesto
Collaborator

Conflicts resolved!

Yes – examples of test cases for the default + overrides are here:

  • Error log default
  • Error log with rotation specified
    • directive: errors errors.txt { rotate_size 2 rotate_age 10 rotate_keep 3 }
    • test case
  • Log default
  • Log with rotation specified
    • directive: log access.log { rotate_size 2 rotate_age 10 rotate_keep 3 }
    • test case
@mholt
Owner
mholt commented Feb 8, 2017 edited

Great work! Thanks @julianvmodesto. I'll merge this now and invite you to be a collaborator to the project so you can help by reviewing PRs, merging accepted ones, and contributing fixes on branches instead of forks. Really appreciate your contribution!

If you want to update the docs, that'll be at https://github.com/caddyserver/caddyserver.com -- or I can update them at next release.

@mholt mholt merged commit ce7d3db into mholt:master Feb 8, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@julianvmodesto julianvmodesto referenced this pull request in caddyserver/caddyserver.com Feb 8, 2017
Merged

Document updated log rolling directives + defaults #110

@julianvmodesto julianvmodesto deleted the unknown repository branch Feb 8, 2017
@mastercactapus mastercactapus added a commit to mastercactapus/caddy that referenced this pull request Feb 11, 2017
@julianvmodesto @mastercactapus julianvmodesto + mastercactapus Roll all logs by default (#1379)
* Use new subdirectives and flatten rolling config

* Set default rotate config

* Set default rolling config (hopefully) errwhere

* Make private

* Flatten errors directive and remove c.IncrNest()

* Don't skip first error log roller subdirective we see

* Remove hadBlock

* Try lumberjack import

* Unname import
0f2c9b5
@wendigo
Collaborator
wendigo commented Feb 20, 2017

IncrNest was removed from Controller which breaks caddy filter building :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment