Skip to content

[pluggable-validation / 1]: protoize conflict detection #169

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

Conversation

klihub
Copy link
Member

@klihub klihub commented May 14, 2025

This PR is the first in a stack which splits up #163 into multiple PRs, which collectively implement configurable restrictions using the proposed pluggable validation API and a builtin default validator plugin.

This PR reworks conflict detection using protobuf messages to provide raw information for validation.

/cc @chrishenzie

Copy link
Contributor

@chrishenzie chrishenzie left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub force-pushed the devel/pluggable-validation/protoize-conflict-detection branch from 47f6c6a to 9db059e Compare May 15, 2025 14:30
Rework equality checking for unit test results using go-cmp,
aiming for fewer kludges. Define a set of Strip() helpers to
reduce otherwise semantically equivalent adjustments/updates
to a unique canonical form and thus allowing equality checks
using go-cmp/cmp.Equal().

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/pluggable-validation/protoize-conflict-detection branch 2 times, most recently from cece288 to 2385a8a Compare May 15, 2025 15:13
Redefine conflict detection data types using protobuf. This allows
passing them to plugins over a ttrpc API.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/pluggable-validation/protoize-conflict-detection branch from 2385a8a to d9d8be8 Compare May 15, 2025 15:17
@github-project-automation github-project-automation bot moved this to Needs Triage in Pull Request Review May 15, 2025
@samuelkarp samuelkarp moved this from Needs Triage to Needs Reviewers in Pull Request Review May 15, 2025
@klihub
Copy link
Member Author

klihub commented May 15, 2025

@samuelkarp @chrishenzie In the scope of this PR, I can (and like to) take a look at if we could get completely rid of Strip() for adjustments and updates. It should be possible if we change result collection so that, instead of deep-initializing full skeletal container adjustment and update structs at the beginning, it would initialize on demand the necessary structure bits as and when needed.

@samuelkarp
Copy link
Member

In the scope of this PR, I can (and like to) take a look at if we could get completely rid of Strip() for adjustments and updates.

If you're okay with it, my preference would be to merge this as-is to unblock the other two PRs, and then handle that in a follow-up.

@klihub
Copy link
Member Author

klihub commented May 15, 2025

In the scope of this PR, I can (and like to) take a look at if we could get completely rid of Strip() for adjustments and updates.

If you're okay with it, my preference would be to merge this as-is to unblock the other two PRs, and then handle that in a follow-up.

Yes, of course. That's also fine.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LTGM

It should be possible if we change result collection so that, instead of deep-initializing full skeletal container adjustment and update structs at the beginning, it would initialize on demand the necessary structure bits as and when needed.

SGTM +++1 for follow-up

@github-project-automation github-project-automation bot moved this from Needs Reviewers to Review In Progress in Pull Request Review May 16, 2025
@mikebrow mikebrow merged commit 6d0b946 into containerd:main May 16, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from Review In Progress to Done in Pull Request Review May 16, 2025
@klihub klihub deleted the devel/pluggable-validation/protoize-conflict-detection branch May 16, 2025 17:39
@samuelkarp samuelkarp added this to the 1.0 milestone May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants