Skip to content

Conversation

JoelSpeed
Copy link
Member

@JoelSpeed JoelSpeed commented May 12, 2020

Description

Creates a LoggingOptions struct and moves all details required for configuring Logging to this struct, plus a LoggingFlagSet, plus a Default function.

Motivation and Context

This is a step towards having a structured configuration. We can move options internally without breaking the external config while we try and create a structure that we are happy with.

I've added a test that checks the defaulting when no options are given, but this will eventually be covered by the tests in #489 once that is merged

How Has This Been Tested?

It's a refactor of internal code, tests pass so all should be fine.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

Copy link
Contributor

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

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

Should we be adding tests for this code?

CHANGELOG.md Outdated

## Changes since v5.1.1

- [#548](https://github.com/oauth2-proxy/oauth2-proxy/pull/548) Move logging options out of main package (@JoelSpeed)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite correct. Shouldn't it be "Splitting logging options from global options structure"

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebase problems 😅 Will fix

Copy link
Member Author

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Technically it's all covered by the existing tests, but I will make sure to add some tests specifically for the logging validation. Will need to see how easy it is to actually test this though as a lot of the options set private members 🤔

CHANGELOG.md Outdated

## Changes since v5.1.1

- [#548](https://github.com/oauth2-proxy/oauth2-proxy/pull/548) Move logging options out of main package (@JoelSpeed)
Copy link
Member Author

Choose a reason for hiding this comment

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

Rebase problems 😅 Will fix

@JoelSpeed JoelSpeed changed the title Move logging options out of main package Separate logging options out of main options structure May 24, 2020
@JoelSpeed JoelSpeed force-pushed the move-logging-options branch from 68ea6b3 to df47cab Compare May 24, 2020 21:22
@JoelSpeed
Copy link
Member Author

@steakunderscore I've had a look at doing testing for this, ideally I would like to have the configureLogger in the actual logger package (since it's not really validating anything, just configuring), but this lead to many import cycle errors.

It's made me realise that the Options structure and the validation is rather overloaded, we should try to remove the mutation from the validate function and instead, just validate. This would involve removing the private members of the Options struct and instead making those members of the main structure (for now). I'll look into this tomorrow as I think it might simplify a lot of things.

Note, I also added an extra commit onto the end of this because I noticed the exclude paths option was inconsistent with our other options

Copy link
Contributor

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

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

I'm happy for tests to come later. Thanks.

@JoelSpeed
Copy link
Member Author

Rebased to fixup changelog conflict, no other changes, so going to treat this as already approved

@JoelSpeed JoelSpeed merged commit a17c488 into master May 31, 2020
@JoelSpeed JoelSpeed deleted the move-logging-options branch May 31, 2020 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants