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

Add default-server-access-log-off to configmap #852

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Conversation

lucacome
Copy link
Member

Proposed changes

This PR adds a new ConfigMap default-server-access-log-off that enables/disables logging for the default server.
If logging is disabled at the http level, logging for the default server is always disabled.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@Rulox
Copy link
Contributor

Rulox commented Feb 21, 2020

Looks good!, I just have one question.

Do you have any kind of golint rule that transforms the structs order into an alphabetical order or something like that? There are a lot of changes in the structs. We had a discussion around this last year. The way we normally order the structs is putting similar options together. For instance, options that are going to be together in the final NGINX Config.

I am ok with either of those methods to be honest, but I just think we should all follow the same rules instead of having some structs one way and some the other.

Let me know your thoughts (also @Dean-Coakley @pleshakov)

@lucacome
Copy link
Member Author

No automatic rule, I just think it's easier to find stuff when it's in alphabetical order. 🙂
When I mentioned this to @pleshakov he didn't say anything about a convention, but I'm happy to change it back.

@Rulox
Copy link
Contributor

Rulox commented Feb 21, 2020

Nah it´s fine. I proposed this last year and I remember we decided not to do it (not sure why). But I am ok with it, to me is easier this way too, :shipit:

@lucacome lucacome added the change Pull requests that introduce a change label Feb 21, 2020
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

🥇

@lucacome lucacome merged commit 40268f7 into master Feb 24, 2020
@lucacome lucacome deleted the default-server-logs branch February 24, 2020 22:33
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants