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

log manager options #165

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

everettraven
Copy link
Contributor

Description of the change

Add logging of the manager options for both the helm and hybrid operator commands

Motivation for the change

resolves #24

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Comment on lines 136 to 137
"LeaderElectionId", options.LeaderElectionID,
"LeaderElectionNamespace", options.LeaderElectionNamespace)
Copy link
Member

Choose a reason for hiding this comment

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

Will it be confusing for the logs to include output for unset or unused flags?

For example, this is going to output something like: LeaderElectionId="", LeaderElectionNamespace="" even if leader election is disabled. Is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I think if leader election is disabled it is likely not important to log those values. I will work on making it so those values are only logged if they are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of commit 37c3d87 it should only show the options that have been specified via the flags or have a default value.

The output will now look like this with both leader election related flags:

{"level":"info","ts":1652113560.2827275,"logger":"cmd","msg":"Setting manager options:","Options":{"HealthProbeAddress":":8081","LeaderElection":true,"LeaderElectionId":"1","LeaderElectionNamespace":"test","MetricsBindAddress":":8080"}}

With only one:

{"level":"info","ts":1652113924.1076055,"logger":"cmd","msg":"Setting manager options:","Options":{"HealthProbeAddress":":8081","LeaderElection":true,"LeaderElectionId":"1","MetricsBindAddress":":8080"}}

With neither:

{"level":"info","ts":1652113954.4604402,"logger":"cmd","msg":"Setting manager options:","Options":{"HealthProbeAddress":":8081","LeaderElection":false,"MetricsBindAddress":":8080"}}

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@coveralls
Copy link

coveralls commented May 9, 2022

Pull Request Test Coverage Report for Build 2295498115

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.616%

Totals Coverage Status
Change from base Build 2260271589: 0.0%
Covered Lines: 1562
Relevant Lines: 1743

💛 - Coveralls

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2022
@everettraven everettraven merged commit 0cb815d into operator-framework:main Jun 1, 2022
@everettraven everettraven deleted the feat/log-flags branch June 1, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/main-binary lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log manager setup done with flags
4 participants