Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Add ability to set log level and enable json logging for all consul-k8s commands #980

Merged
merged 17 commits into from
Jun 30, 2021
Merged

Add ability to set log level and enable json logging for all consul-k8s commands #980

merged 17 commits into from
Jun 30, 2021

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Jun 7, 2021 •
edited
Loading

Changes proposed in this PR:

  • Add ability to set json logging for consul-k8s components : global.logJSON
  • Add ability to control log level for all consul-k8s components global.logLevel

NOTES:

  • Envoy + consul server/client are not currently implemented.

How I've tested this PR:
unit tests, acceptance tests pass

How I expect reviewers to test this PR:
code review, acceptance tests pass
Also have manually testing using steps in hashicorp/consul-k8s#523

Checklist:

  • Bats tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@kschoche kschoche self-assigned this Jun 7, 2021
@kschoche kschoche changed the title Add json logging ability and specify log-level to all consul-k8s commands [draft] Add ability to set log level and enable json logging for all consul-k8s commands Jun 22, 2021
@kschoche kschoche marked this pull request as ready for review June 22, 2021 18:30
@kschoche kschoche requested review from a team, lkysow and ndhanushkodi and removed request for a team June 22, 2021 22:48
@kschoche kschoche mentioned this pull request Jun 22, 2021
Merged
2 tasks
kschoche
kschoche commented Jun 22, 2021
View reviewed changes
.circleci/config.yml Outdated Show resolved Hide resolved
@kschoche kschoche added the enhancement New feature or request label Jun 22, 2021
lkysow
lkysow reviewed Jun 28, 2021
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looks god. One question about the move to sh -ec

templates/connect-inject-deployment.yaml Outdated Show resolved Hide resolved
templates/controller-deployment.yaml Show resolved Hide resolved
templates/create-federation-secret-job.yaml Outdated Show resolved Hide resolved
templates/ingress-gateways-deployment.yaml Outdated Show resolved Hide resolved
templates/ingress-gateways-deployment.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
ndhanushkodi
ndhanushkodi reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

For a command like consul-sidecar for connect-injected pods that doesn't have its logLevel/logJSON set here, do you need to add something to the consul-k8s PR to invoke that internal command with the right flag based on the global value?

ndhanushkodi
ndhanushkodi approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Nice work!!

@lkysow
Copy link
Member

lkysow commented Jun 29, 2021

Should we have a follow-up PR that sets the log level/json for Consul clients/servers too?

@kschoche
Copy link
Contributor Author

Should we have a follow-up PR that sets the log level/json for Consul clients/servers too?

Yup this was only for subcommands.

@kschoche kschoche merged commit 5ae418b into master Jun 30, 2021
@kschoche kschoche deleted the logging-updates-json branch June 30, 2021 01:28
@TJM TJM mentioned this pull request Jun 30, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

@lkysow lkysow lkysow approved these changes

Assignees

@kschoche kschoche

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kschoche @lkysow @ndhanushkodi