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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support context propagation without breaking changes #943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

goccy
Copy link

@goccy goccy commented May 15, 2024

This PR is related to #925 .

First of all, thank you for the review the other day.
As suggested, I tried to avoid changing the public interface and instead added new types to support it. Although there are many changes, as you can see from the test code, I added the functionality without altering the existing behavior 馃帀

Additionally, during the changes, it was unnecessary to pass context.Context to AddQualifierContext / QualifyContext / QualifyIfPresentContext, so if there are any issues, it's possible to revert these to use the original types. Please let me know your thoughts.

Pull Requests Guidelines

See CONTRIBUTING.md for more details about when to create
a GitHub Pull Request and when other kinds of contributions or
consultation might be more desirable.

When creating a new pull request, please fork the repo and work within a
development branch.

Commit Messages

  • Most changes should be accompanied by tests.
  • Commit messages should explain why the changes were made.
Summary of change in 50 characters or less

Background on why the change is being made with additional detail on
consequences of the changes elsewhere in the code or to the general
functionality of the library. Multiple paragraphs may be used, but
please keep lines to 72 characters or less.

Reviews

  • Perform a self-review.
  • Make sure the Travis CI build passes.
  • Assign a reviewer once both the above have been completed.

Merging

  • If a CEL maintaner approves the change, it may be merged by the author if
    they have write access. Otherwise, the change will be merged by a maintainer.
  • Multiple commits should be squashed before merging.
  • Please append the line closes #<issue-num>: description in the merge message,
    if applicable.

- To avoid breaking changes, add a type to support context
@goccy goccy marked this pull request as ready for review May 15, 2024 13:02
@TristonianJones
Copy link
Collaborator

@goccy thanks for taking another look at this. I'm sorry I haven't been able to review the change yet. It's a large change and I haven't had a sufficiently large block of time to review it in the manner it deserves to be. I will try to get to the review in more detail next week.

@TristonianJones
Copy link
Collaborator

@goccy I think I'm going to revive #368 as it will have fewer impacts and potential for breakages while still giving you most of what you want. The difference is that the implementation won't block on concurrent calls unless it needs to, instead iterating through calls in batches and re-evaluating. It's a definite overhead, but the highest cost should be the I/O rather than the repeated evaluation.

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

2 participants