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

Feature/flag include year #2884

Merged
merged 15 commits into from
Jul 27, 2022
Merged

Feature/flag include year #2884

merged 15 commits into from
Jul 27, 2022

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Jul 25, 2022

Proposed changes

This change introduces a new flag to the ingress controller -include-year which will add the year to the log header when set. The default value of this flag is false

Deprecation notice

This flag is planned to be remove from the ingress controller by release 2.7 at which point all logs from the ingress controller will display the year by default.

Checklist

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

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels Jul 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #2884 (30fa0db) into main (99fba9e) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2884      +/-   ##
==========================================
+ Coverage   53.02%   53.03%   +0.01%     
==========================================
  Files          58       58              
  Lines       15641    15645       +4     
==========================================
+ Hits         8294     8298       +4     
- Misses       7068     7070       +2     
+ Partials      279      277       -2     
Impacted Files Coverage Δ
cmd/nginx-ingress/flags.go 25.92% <0.00%> (-0.66%) ⬇️
internal/k8s/configuration.go 95.76% <0.00%> (+0.36%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@shaun-nx shaun-nx marked this pull request as ready for review July 26, 2022 13:02
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

👍🏻 great job!

@@ -65,6 +65,7 @@ spec:
- -nginx-plus
- -nginx-configmaps=$(POD_NAMESPACE)/nginx-config
- -default-server-tls-secret=$(POD_NAMESPACE)/default-server-secret
#- -include-year
Copy link
Contributor

Choose a reason for hiding this comment

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

can specify the same in deployments/daemon-set/nginx-plus-ingress.yaml as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I will also add it to the OSS version of the deployments

@@ -178,6 +178,9 @@ controller:
## Enable OIDC policies.
enableOIDC: false

## Include year in log header. This parameter will be removed in release 2.7 and the year will be included by default.
includeYear: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand this. None of the values in this file start with controller. why would the includeYear value need to start with that?

Copy link
Contributor

@vepatel vepatel Jul 26, 2022

Choose a reason for hiding this comment

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

it refers to https://github.com/nginxinc/kubernetes-ingress/blob/feature/flag-include-year/deployments/helm-chart/values.yaml#L1 so when you're installing using helm-chart helm install my-release --set controller.kind=daemonset --set controller.nginxplus=true --set controller.image.repository=nginx-ingress --set controller.image.tag=edge --set controller.includeYear=true ......

@shaun-nx shaun-nx requested a review from vepatel July 26, 2022 15:25
@shaun-nx shaun-nx merged commit 06ec33b into main Jul 27, 2022
@shaun-nx shaun-nx deleted the feature/flag-include-year branch July 27, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants