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

Reuse context in reconciler #87

Merged
merged 3 commits into from
Jun 4, 2021
Merged

Reuse context in reconciler #87

merged 3 commits into from
Jun 4, 2021

Conversation

DiogoMCampos
Copy link
Contributor

@DiogoMCampos DiogoMCampos commented May 24, 2021

Fixes #67.

Two questions:

  • I haven't touched requestsForApplicationChange nor requestsForSecretChange as they are called in handler.EnqueueRequestsFromMapFunc(r.requestsForApplicationChange) and handler.EnqueueRequestsFromMapFunc(r.requestsForSecretChange) respectively. Should we?
  • I'm reusing the global ctx in tests - is there any better approach?

Copy link
Collaborator

@sledigabel sledigabel left a comment

Choose a reason for hiding this comment

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

LGTM, but do we know where is the ctx for the Reconcile function coming from and who sets it?
the logic being, it might be a context that has nothing to do with the actual context we want propagated (in anticipation of #63)

@sledigabel
Copy link
Collaborator

Fixes #67.

Two questions:

  • I haven't touched requestsForApplicationChange nor requestsForSecretChange as they are called in handler.EnqueueRequestsFromMapFunc(r.requestsForApplicationChange) and handler.EnqueueRequestsFromMapFunc(r.requestsForSecretChange) respectively. Should we?
  • I'm reusing the global ctx in tests - is there any better approach?

OK So THAT is the answer to my question, the context is initialised there :-)
I think it's ok we can leave that like that for now.

@maruina could prob confirm this.

@maruina
Copy link
Contributor

maruina commented Jun 2, 2021

I think we should change requestsForApplicationChange and requestsForSecretChange as they define they own context (see for example https://github.com/Skyscanner/applicationset-progressive-sync/pull/87/files#diff-a853c97824440aa3801077e1f5661f9087e27091372140f15f89ed5c8d0c5ba6R247)

Regarding the test, I'm not sure about that so I think you can keep the current ctx.

@sledigabel
Copy link
Collaborator

I think we should change requestsForApplicationChange and requestsForSecretChange as they define they own context

But if they don't generate the context, what will?
It seems to be that this is the context that gets propagated to the Reconciler?
Presumably the context needs to be instantiated somewhere?

@maruina
Copy link
Contributor

maruina commented Jun 2, 2021

I think we should change requestsForApplicationChange and requestsForSecretChange as they define they own context

But if they don't generate the context, what will?
It seems to be that this is the context that gets propagated to the Reconciler?
Presumably the context needs to be instantiated somewhere?

You are correct. I was already thinking in a kubebuilder 3.0 scenario.

@maruina maruina self-requested a review June 2, 2021 20:19
Copy link
Contributor

@maruina maruina left a comment

Choose a reason for hiding this comment

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

LGTM

@maruina maruina enabled auto-merge (squash) June 4, 2021 07:27
@maruina maruina merged commit 7f4bbae into main Jun 4, 2021
@maruina maruina deleted the reuse-context branch August 6, 2021 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass context in reconciliation methods instead of defining a new one
5 participants