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

Use context consistently #165

Merged
merged 3 commits into from
Dec 10, 2021
Merged

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Dec 1, 2021

Eliminate all uses of context.TODO(). Eliminate all uses of
context.Background() in production code. In all cases, either our
callers have a Context they can pass or the k8s framework provides one.

Remove unnecessary uses of context.Context. In quite a few places we
require context.Context, but nothing actually consumes it so it's just an
unused parameter that pollutes call sites and gives the false impression
that these requests are cancellable/etc. I've removed usages of Context
in these cases.

This change breaks gatekeeper - since this modifies our external APIs,
making this in a non-breaking way would require 5 PRs instead of 2. Once
this PR has been submitted, I'll submit a PR which updates the version
of frameworks/ that gatekeeper uses and fix the resulting breakages.

Signed-off-by: Will Beason willbeason@google.com

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021

Codecov Report

Merging #165 (44b36b7) into master (0087992) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   44.60%   44.60%           
=======================================
  Files          54       54           
  Lines        2717     2717           
=======================================
  Hits         1212     1212           
  Misses       1259     1259           
  Partials      246      246           
Flag Coverage Δ
unittests 44.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...y-agent/frameworks/constraint/pkg/client/client.go 66.14% <0.00%> (-0.09%) ⬇️
...-agent/frameworks/constraint/pkg/client/backend.go 68.75% <0.00%> (ø)
...works/constraint/pkg/client/drivers/local/local.go 68.24% <0.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0087992...44b36b7. Read the comment docs.

constraint/pkg/client/client.go Outdated Show resolved Hide resolved
constraint/pkg/client/backend.go Outdated Show resolved Hide resolved
@julianKatz
Copy link
Contributor

Are we confident that this decision can grow with the Constraint Framework? What if we wanted to support another backend? Perhaps a different implementation there would benefit from the ability to cancel requests.

No argument on the lack of utility at the moment, but we should be mindful of changing the public interface. Other projects (like config validator) use the constraint framework as well, and we don't want them to break unless it's for a great reason.

@maxsmythe
Copy link
Contributor

We'll need to change the API as part of sharding Rego evaluation, so WRT backwards compatibility, as long as all these changes go out at once, it's probably not that much more pain.

As far as for future usage, it's a fair concern. IMO we should be able to inject context into requests that can/should be interrupted b/c they can have the potential to run for a long time. Audit() is a good example of this. Easy to cancel (no side effects), and long-running and resource-intensive enough to be worth canceling.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@willbeason
Copy link
Member Author

Are we confident that this decision can grow with the Constraint Framework? What if we wanted to support another backend? Perhaps a different implementation there would benefit from the ability to cancel requests.

No argument on the lack of utility at the moment, but we should be mindful of changing the public interface. Other projects (like config validator) use the constraint framework as well, and we don't want them to break unless it's for a great reason.

Right now I'm tending towards YAGNI - since gatekeeper is the primary consumer, if we end up needing to add context back, I'd rather have to do it in the future and only add it where it's needed, than request it everywhere and just have it in places we end up never needing it.

Will Beason added 2 commits December 6, 2021 09:10
Eliminate all uses of context.TODO(). Eliminate all uses of
context.Background() in production code. In all cases, either our
callers have a Context they can pass or the k8s framework provides one.

This change breaks gatekeeper - since this modifies our external APIs,
making this in a non-breaking way would require 5 PRs instead of 2. Once
this PR has been submitted, I'll submit a PR which updates the version
of frameworks/ that gatekeeper uses and fix the resulting breakages.

Signed-off-by: Will Beason <willbeason@google.com>
There were a lot of Contexts we asked for in interfaces, but didn't
actually use. Since this pollutes call sites and gives the false
impression that these calls are cancellable/etc, we shouldn't have them.

This commit removes these unnecessary contexts.

Signed-off-by: Will Beason <willbeason@google.com>
Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason merged commit eb4524f into open-policy-agent:master Dec 10, 2021
@willbeason willbeason deleted the context branch December 10, 2021 16:55
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.

None yet

4 participants