8000 fix(deps): Updates to major ver of protovalidate by dmihalcik-virtru · Pull Request #2284 · opentdf/platform · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(deps): Updates to major ver of protovalidate #2284

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

Merged
merged 1 commit into from
May 30, 2025

Conversation

dmihalcik-virtru
Copy link
Member
@dmihalcik-virtru dmihalcik-virtru commented May 21, 2025

Proposed Changes

  • This was causing dependabot to fail

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@dmihalcik-virtru dmihalcik-virtru requested review from a team as code owners May 21, 2025 18:41
@github-actions github-actions bot added comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) size/s labels May 21, 2025
Copy link
Contributor
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team,

Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.

This pull request, authored by dmihalcik-virtru, focuses on updating the project's dependencies. Specifically, it updates the protovalidate library to a major new version (v0.12.0). According to the description, this update is necessary because the previous version was causing Dependabot to fail.

The changes involve updating the go.mod and go.sum files to reflect the new dependency version and its transitive dependencies. Additionally, the import paths for the protovalidate library have been updated in several test files to match the new module path.

Highlights

  • Dependency Update: The primary change is the update of the protovalidate dependency from github.com/bufbuild/protovalidate-go v0.9.3 to buf.build/go/protovalidate v0.12.0.
  • Fix Dependabot Issue: This update is intended to resolve a failure encountered with Dependabot.
  • Import Path Changes: The import path for the protovalidate library has been updated from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate in several test files.
  • Indirect Dependency Updates: Several indirect dependencies, including cel.dev/expr, github.com/google/cel-go, and the generated bufbuild/protovalidate protobuf code, were also updated as a result of the main dependency change.

Changelog

Click here to see the changelog
  • service/go.mod
    • Removed direct dependency github.com/bufbuild/protovalidate-go v0.9.3 (line 15).
    • Added direct dependency buf.build/go/protovalidate v0.12.0 (line 8).
    • Updated indirect dependency cel.dev/expr from v0.19.1 to v0.23.1 (line 56).
    • Added indirect dependency github.com/bufbuild/protovalidate-go v0.9.1 (line 58).
    • Updated indirect dependency buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go to a newer timestamped version (line 63).
    • Updated indirect dependency github.com/google/cel-go from v0.24.1 to v0.25.0 (line 113).
  • service/go.sum
    • Updated checksums corresponding to the dependency changes in go.mod (lines 1-6, 51-52, 162-163).
  • service/policy/actions/actions_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 7).
  • service/policy/attributes/attributes_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 8).
  • service/policy/kasregistry/key_access_server_registry_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 7).
  • service/policy/keymanagement/key_management_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 6).
  • service/policy/namespaces/namespaces_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 6).
  • service/policy/registeredresources/registered_resources_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 7).
  • service/policy/resourcemapping/resource_mapping_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 6).
  • service/policy/subjectmapping/subject_mapping_test.go
    • Updated import path for protovalidate from github.com/bufbuild/protovalidate-go to buf.build/go/protovalidate (line 6).
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Dependencies shift and sway,
Major versions light the way.
Fixing bots with code so neat,
Making builds feel quite complete.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This PR updates protovalidate to a major version (from github.com/bufbuild/protovalidate-go v0.9.3 to buf.build/go/protovalidate v0.12.0) and adjusts import paths accordingly. The changes in go.mod, go.sum, and the test file import paths appear to correctly implement this dependency update.

The primary concern with a major version upgrade, especially for a validation library like protovalidate, is the potential for breaking changes or subtle alterations in behavior that could impact existing functionality. The PR description includes a checklist for testing (unit tests, integration tests) which are currently unchecked.

It is crucial to ensure that comprehensive testing has been performed and that all tests pass with this new version. This includes verifying:

  1. Existing validations continue to work as expected.
  2. No new, unexpected validation errors are introduced.
  3. If protovalidate is used in performance-critical paths, that performance has not regressed.

Could you please update the PR description or comment on the status of the test suite with these changes? Specifically, confirming that all relevant tests pass or detailing any necessary updates made to tests due to legitimate changes in the new protovalidate version.

Summary of Findings

  • Testing Confirmation for Major Version Upgrade: The PR involves a major version upgrade of the protovalidate library. The checklist in the PR description for unit and integration tests is unchecked. It's critical to confirm that all tests pass with the new version to ensure no regressions or unexpected behavior changes have been introduced. This is a high-priority concern before merging.

Merge Readiness

The dependency update and import path changes in this PR are mechanically correct. However, due to the nature of a major version upgrade for a critical library like protovalidate, and the currently unchecked testing checklist in the PR description, I recommend that this PR not be merged until it's confirmed that all relevant unit and integration tests pass with these changes. Addressing this testing confirmation is crucial to maintain code quality and prevent potential regressions. I am unable to approve the pull request myself; please ensure further review and approval by others once the testing status is clarified.

Copy link
Contributor
Benchmark results, click to expand

Benchmark Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 629.789302ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 362.441877ms
Throughput 275.91 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 1m16.691772523s
Average Latency 765.07726ms
Throughput 65.20 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4949
Failed Requests 51
Concurrent Requests 50
Total Time 1m9.009704248s
Average Latency 686.540711ms
Throughput 71.71 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
51 occurrences

Standard Benchmark Metrics Skipped or Failed

elizabethhealy
elizabethhealy previously approved these changes May 21, 2025
@dmihalcik-virtru
Copy link
Member Author

Needs a re-review because I somehow decided to also fix the stray direct dep in the indirect block which is causing more merge conflicts

Copy link
Contributor
Benchmark results, click to expand

Benchmark Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 523.788036ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 383.917695ms
Throughput 260.47 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 1m16.495117164s
Average Latency 763.134585ms
Throughput 65.36 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4939
Failed Requests 61
Concurrent Requests 50
Total Time 1m8.079728303s
Average Latency 677.477212ms
Throughput 72.55 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
61 occurrences

Standard Benchmark Metrics Skipped or Failed

strantalis
strantalis previously approved these changes May 23, 2025
This was causing dependabot to fail
Updating this means we need a new connect too
Also, resorts indirect and direct service go deps
Copy link
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 469.143653ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 301.460411ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 342.776875ms
Throughput 291.73 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 53.76456686s
Average Latency 535.658711ms
Throughput 93.00 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4947
Failed Requests 53
Concurrent Requests 50
Total Time 44.417259116s
Average Latency 439.809465ms
Throughput 111.38 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
53 occurrences

Standard Benchmark Metrics Skipped or Failed

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue May 30, 2025
Merged via the queue into main with commit 39ad3c9 May 30, 2025
28 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the DSPX-1109-fix-gomod branch May 30, 2025 18:15
jakedoublev pushed a commit that referenced this pull request May 30, 2025
### Proposed Changes

* This was causing dependabot to fail


### Checklist

- [x] I have added or updated unit tests
- [x] I have added or updated integration tests (if appropriate)
- [x] I have added or updated documentation

### Testing Instructions
github-merge-queue bot pushed a commit that referenced this pull request Jun 3, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.5.5](service/v0.5.4...service/v0.5.5)
(2025-05-30)


### Features

* adds basic config root key manager
([#2303](#2303))
([dd0d22f](dd0d22f))
* **policy:** cache SubjectConditionSet selectors in dedicated column
maintained via trigger
([#2320](#2320))
([215791f](215791f))
* **policy:** map and merge grants and keys
([#2324](#2324))
([abf770f](abf770f))


### Bug Fixes

* **deps:** bump github.com/opentdf/platform/sdk from 0.4.5 to 0.4.7 in
/service in the internal group
([#2334](#2334))
([7f5a182](7f5a182))
* **deps:** Updates to major ver of protovalidate
([#2284](#2284))
([39ad3c9](39ad3c9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0