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

Support URL CEL library #244

Merged
merged 12 commits into from
Oct 2, 2024
Merged

Support URL CEL library #244

merged 12 commits into from
Oct 2, 2024

Conversation

pikachu0310
Copy link
Collaborator

@pikachu0310 pikachu0310 commented Sep 17, 2024

Provide functionalities similar to Go's net/url using the grpc.federation.url library.

  • impl url.parse
  • impl URL functions
  • impl Userinfo functions
  • impl other functions
  • add url.parse test
  • add URL functions test
  • add Userinfo functions test
  • add other functions test
  • write docs/cel/url.md

This comment has been minimized.

This comment has been minimized.

@pikachu0310 pikachu0310 marked this pull request as ready for review September 26, 2024 10:08
Copy link
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

I left some comments, so PTAL 🙏

grpc/federation/cel/url.go Outdated Show resolved Hide resolved
grpc/federation/cel/url.go Outdated Show resolved Hide resolved
grpc/federation/cel/url.go Outdated Show resolved Hide resolved
grpc/federation/cel/url.go Outdated Show resolved Hide resolved

This comment has been minimized.

@pikachu0310
Copy link
Collaborator Author

I have addressed ktgw's review comments and resolved the conflicts. Please review it again.

Copy link
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

I left some comments so PTAL 🙏

grpc/federation/cel/url.go Outdated Show resolved Hide resolved
grpc/federation/cel/url.go Outdated Show resolved Hide resolved
grpc/federation/cel/url.go Outdated Show resolved Hide resolved

This comment has been minimized.

Copy link
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

LGTM 🙆‍♂️

grpc/federation/cel/url.go Outdated Show resolved Hide resolved
also lib.refToGoUserinfoValue
@pikachu0310 pikachu0310 requested a review from utahta October 1, 2024 10:12
Copy link

github-actions bot commented Oct 1, 2024

Code Metrics Report

main (e478944) #244 (4e0c688) +/-
Coverage 66.0% 66.3% +0.3%
Code to Test Ratio 1:0.4 1:0.4 +0.0
Test Execution Time 7m55s 7m45s -10s
Details
  |                     | main (e478944) | #244 (4e0c688) |  +/-  |
  |---------------------|----------------|----------------|-------|
+ | Coverage            |          66.0% |          66.3% | +0.3% |
  |   Files             |             75 |             76 |    +1 |
  |   Lines             |          12418 |          12557 |  +139 |
+ |   Covered           |           8192 |           8320 |  +128 |
+ | Code to Test Ratio  |          1:0.4 |          1:0.4 |  +0.0 |
  |   Code              |          38524 |          39019 |  +495 |
+ |   Test              |          13878 |          14665 |  +787 |
+ | Test Execution Time |          7m55s |          7m45s |  -10s |

Code coverage of files in pull request scope (100.0% → 93.0%)

Files Coverage +/-
grpc/federation/cel/lib.go 100.0% 0.0%
grpc/federation/cel/url.go 92.1% +92.1%

Reported by octocov

Copy link
Member

@utahta utahta left a comment

Choose a reason for hiding this comment

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

LGTM!

@shuheiktgw shuheiktgw merged commit 04088ed into main Oct 2, 2024
4 checks passed
@shuheiktgw shuheiktgw deleted the net-url branch October 2, 2024 01:34
pikachu0310 added a commit that referenced this pull request Oct 4, 2024
Support strconv CEL library #244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants