Closes #99: Make logging configurable. #100

fgiloux · 2022-01-25T05:33:30Z

Description of the change:

Closes #99
This pull request aligns the logging configuration in the prune feature with what is done in the rest of the library. It declares a package level variable for a logger. The logger is retrieved from the controller-runtime library, which is leveraging logr.

Motivation for the change:

The current prune implementation could not be used outside of the prune package as the log field of the Config struct was unexported.

A pull request for amending the documentation will follow.

openshift-ci · 2022-01-25T05:33:39Z

Hi @fgiloux. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fgiloux · 2022-01-25T05:34:20Z

cc @camilamacedo86 @Sijoma @ryantking

fgiloux · 2022-01-26T06:10:55Z

@camilamacedo86 can you please review the PR?

camilamacedo86

/lgtm

camilamacedo86 · 2022-01-27T00:31:01Z

/ok-to-test

coveralls · 2022-01-27T00:32:37Z

Pull Request Test Coverage Report for Build 2052966183

22 of 24 (91.67%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 80.89%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
prune/prune.go	12	14	85.71%

Totals
Change from base Build 1710189297:	-0.01%
Covered Lines:	618
Relevant Lines:	764

💛 - Coveralls

fgiloux · 2022-01-28T15:33:34Z

@theishshah @varshaprasad96 can you please take a look? I would very welcome if we can get it merged soon.
Thanks!

fgiloux · 2022-03-21T16:41:15Z

@theishshah @varshaprasad96 Anything preventing the merge of this PR?

joelanford · 2022-03-21T16:49:13Z

@fgiloux I made this comment over on the issue. WDYT?

varshaprasad96

/lgtm
/approve
/hold
holding for @joelanford's lgtm based on the last comment.

openshift-ci · 2022-03-22T21:08:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: varshaprasad96

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [varshaprasad96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fgiloux · 2022-03-23T19:18:19Z

@joelanford @varshaprasad96 @camilamacedo86 reworked to address Joe's concerns

joelanford · 2022-03-23T19:22:40Z

prune/prune.go

+// Note that there is no compile time check whether a logger can be retrieved by either way.
+// keysAndValues allow to add fields to the logs, cf logr documentation.
+func Logger(ctx context.Context, cfg Config, keysAndValues ...interface{}) logr.Logger {
+	log := cfg.Log


I kind of feel like cfg.Log (if it's set) should win over logr.FromContext(). WDYT?

My thinking was that cfg.Log is set once whereas logr.FromContext() can be changed, although unlikely with every request. I picked the one with the scope having the shorter lifetime

My thought was that if I go to the trouble of setting the logger in the config, that's an extra explicit step I'm taking to say "use this logger". With the code as is, if I was using this in Reconcile, I'd have to create a new context that drops the logger from the Reconcile context before using the prune functions. That seems pretty awkward.

joelanford · 2022-03-23T19:24:16Z

prune/prune.go

+func Logger(ctx context.Context, cfg Config, keysAndValues ...interface{}) logr.Logger {
+	log := cfg.Log
+	if ctx != nil {
+		if logger, err := logr.FromContext(ctx); err == nil {


Should we use controller-runtime's version of this function so that if there isn't a logger in the context, we end up with the controller-runtime's global logger? Or is that too much coupling to controller-runtime?

My thinking was to avoid coupling with controller-runtime... but the library has already a dependency on controller-runtime. I think Jeff wanted to make it available to other frameworks, hence

type Config struct { Clientset kubernetes.Interface // kube client used by pruning

Due to the already existing dependency I can use the controller-runtime version

This also means that we will always get a logger from context. In that case this cannot have precedence over the logger set in the config struct otherwise this would never be set.

joelanford · 2022-03-23T19:24:32Z

prune/prune.go

@@ -181,3 +181,17 @@ func (config Config) validate() (err error) {
 	}
 	return nil
 }
+
+// Logger returns a logger from the context using logr method or Config.Log if none is found
+// controller-runtime automatically provides a logger in context.Context.


Suggested change

// controller-runtime automatically provides a logger in context.Context.

// controller-runtime automatically provides a logger in context.Context during Reconcile calls.

Maybe other times as well? This is the only one I know about though.

I won't debate on that :-)

joelanford · 2022-03-23T19:26:45Z

prune/prune.go

+// controller-runtime automatically provides a logger in context.Context.
+// Note that there is no compile time check whether a logger can be retrieved by either way.
+// keysAndValues allow to add fields to the logs, cf logr documentation.
+func Logger(ctx context.Context, cfg Config, keysAndValues ...interface{}) logr.Logger {


Does this function need to be exported?

It is exported in controller-runtime and the user can leverage it for custom code that gets injected through PreDeleteHook and CustomStrategy. I am passing the context to them for the purpose.

Got it. Makes sense. Thanks for the explanation!

joelanford · 2022-03-24T14:06:51Z

/lgtm

…xt (controller-runtime way) Signed-off-by: Frederic Giloux <[email protected]>

camilamacedo86 · 2022-03-29T01:23:34Z

Hi @joelanford @jmrodri @varshaprasad96

This project is still < 1.0.0, any reason for we do not move forward when the api diff fails?
Do we need to ensure backwards compatibility in the lib? I do not think so, but before we click on the button I'd like just to confirm. :-)

c/c @fgiloux

joelanford · 2022-03-29T02:28:09Z

Do we know what the impact would be if this broke (what change is necessary for callers?, how many operators are using the library?). I'd say its okay to break this if necessary, but we should generally try to avoid it.

Did someone mention that @ryantking had a larger design or some follow-on changes to make? If so, I'd suggest that we try to avoid breaking these APIs twice.

fgiloux · 2022-03-29T04:26:00Z

@camilamacedo86 @joelanford as per #99 the current situation is that Config.log is not exported and not set automatically but it is called in the code. In my opinion this makes the library broken. Hence there is nothing to break. As the prune library is documented in multiple places the urgency is to make it usable That's the aim of this PR.
To answer Joe''s questions:

Do we know what the impact would be if this broke (what change is necessary for callers?

No impact, adding a field, which is optional is not breaking an API. This means if the library had happened to work it would work further but it did not by any way.

how many operators are using the library?). I'd say its okay to break this if necessary, but we should generally try to avoid it.

As log is called and not configurable my guess would be exactly 0.

Did someone mention that @ryantking had a larger design or some follow-on changes to make? If so, I'd suggest that we try to avoid breaking these APIs twice.

I mentioned that. The EP has been however stale waiting to get merged for months. The urgency is to make a documented library usable.

joelanford · 2022-03-29T12:54:33Z

Seems like the breaking change is adding a context parameter to the PreDelete and StrategyFunc function signatures.

Based on everything @fgiloux said, it seems like a no brainer to get this library to a working state even if that means some minor breaking API changes.

/lgtm

camilamacedo86 · 2022-03-30T10:14:39Z

/lgtm

openshift-ci bot requested review from theishshah and varshaprasad96 January 25, 2022 05:33

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 25, 2022

This was referenced Jan 25, 2022

Documents operator-lib PR #100. operator-framework/operator-sdk#5515

Merged

Improve prune feature implementation #101

Closed

camilamacedo86 reviewed Jan 27, 2022

View reviewed changes

openshift-ci bot assigned camilamacedo86 Jan 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2022

camilamacedo86 approved these changes Jan 27, 2022

View reviewed changes

varshaprasad96 approved these changes Mar 22, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2022

openshift-ci bot assigned varshaprasad96 Mar 22, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2022

fgiloux force-pushed the main branch from c065097 to f8bce33 Compare March 23, 2022 19:15

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2022

joelanford reviewed Mar 23, 2022

View reviewed changes

fgiloux force-pushed the main branch from f8bce33 to b0bec4a Compare March 24, 2022 09:00

openshift-ci bot assigned joelanford Mar 24, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022

fgiloux changed the title ~~Closes #99: Remove log from Config and align with the library strategy.~~ Closes #99: Make logging configurable. Mar 28, 2022

Export Config.Log and make possible to configure logger through conte…

daea40b

…xt (controller-runtime way) Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the main branch from b0bec4a to daea40b Compare March 28, 2022 14:36

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

camilamacedo86 merged commit d389ad4 into operator-framework:main Mar 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #99: Make logging configurable. #100

Closes #99: Make logging configurable. #100

fgiloux commented Jan 25, 2022

openshift-ci bot commented Jan 25, 2022

fgiloux commented Jan 25, 2022

fgiloux commented Jan 26, 2022

camilamacedo86 left a comment

camilamacedo86 commented Jan 27, 2022

coveralls commented Jan 27, 2022 •

edited

Loading

fgiloux commented Jan 28, 2022

fgiloux commented Mar 21, 2022

joelanford commented Mar 21, 2022

varshaprasad96 left a comment

openshift-ci bot commented Mar 22, 2022

fgiloux commented Mar 23, 2022

joelanford Mar 23, 2022

fgiloux Mar 24, 2022

joelanford Mar 24, 2022

joelanford Mar 23, 2022

fgiloux Mar 24, 2022

fgiloux Mar 24, 2022

joelanford Mar 23, 2022

fgiloux Mar 24, 2022

joelanford Mar 23, 2022

fgiloux Mar 24, 2022

joelanford Mar 24, 2022

joelanford commented Mar 24, 2022

camilamacedo86 commented Mar 29, 2022 •

edited

Loading

joelanford commented Mar 29, 2022

fgiloux commented Mar 29, 2022

joelanford commented Mar 29, 2022

camilamacedo86 commented Mar 30, 2022

	// controller-runtime automatically provides a logger in context.Context.
	// controller-runtime automatically provides a logger in context.Context during Reconcile calls.

Closes #99: Make logging configurable. #100

Closes #99: Make logging configurable. #100

Conversation

fgiloux commented Jan 25, 2022

openshift-ci bot commented Jan 25, 2022

fgiloux commented Jan 25, 2022

fgiloux commented Jan 26, 2022

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Jan 27, 2022

coveralls commented Jan 27, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2052966183

💛 - Coveralls

fgiloux commented Jan 28, 2022

fgiloux commented Mar 21, 2022

joelanford commented Mar 21, 2022

varshaprasad96 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 22, 2022

fgiloux commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Mar 24, 2022

camilamacedo86 commented Mar 29, 2022 • edited Loading

joelanford commented Mar 29, 2022

fgiloux commented Mar 29, 2022

joelanford commented Mar 29, 2022

camilamacedo86 commented Mar 30, 2022

coveralls commented Jan 27, 2022 •

edited

Loading

camilamacedo86 commented Mar 29, 2022 •

edited

Loading