pkg/rest: refactor and simplify #292

kolyshkin · 2025-03-31T18:35:09Z

This removes unused code from pkg/rest, and simplifies it.

Should not change functionality in any way.

Please see individual commits for details.

Summary by Sourcery

Refactor and simplify the REST package by removing unused code and consolidating URI parsing logic

Enhancements:

Simplified URI parsing logic by merging validation steps into the NewEndpoint function
Removed case-insensitive scheme parsing and consolidated scheme conversion

Tests:

Updated test cases to reflect new Endpoint creation and scheme parsing logic

Chores:

Removed unused functions like validateRestfulURI and parseRestfulURI
Removed unused import for cmdline package

sourcery-ai · 2025-03-31T18:35:14Z

Reviewer's Guide by Sourcery

Refactored the REST package by removing unused code, simplifying URI parsing, and updating tests to reflect these changes. The URI parsing and validation logic has been consolidated into the NewEndpoint function. Unnecessary functions and case-insensitive scheme parsing were removed.

Sequence diagram for Endpoint creation

sequenceDiagram
  participant Client
  participant NewEndpoint
  participant url.ParseRequestURI
  participant toRestScheme

  Client->>NewEndpoint: NewEndpoint(input)
  NewEndpoint->>url.ParseRequestURI: url.ParseRequestURI(input)
  url.ParseRequestURI-->>NewEndpoint: Returns URI
  NewEndpoint->>toRestScheme: toRestScheme(URI.Scheme)
  toRestScheme-->>NewEndpoint: Returns ServiceScheme
  NewEndpoint-->>Client: Returns Endpoint, error

Updated class diagram for Endpoint

classDiagram
  class Endpoint {
    Host: string
    Path: string
    Scheme: ServiceScheme
  }

  class ServiceScheme {
    None
    Unix
    TCP
  }

  Endpoint -- ServiceScheme : has a

File-Level Changes

Change	Details	Files
Consolidated URI parsing and validation logic into the `NewEndpoint` function.	Merged the URI parsing and validation steps into the `NewEndpoint` function. Replaced direct `url.ParseRequestURI` calls with calls to `NewEndpoint`.	`pkg/rest/rest.go` `pkg/rest/rest_test.go`
Removed unused code and simplified scheme parsing.	Removed the `validateRestfulURI` and `parseRestfulURI` functions. Removed case-insensitive scheme parsing in `toRestScheme`. Removed unused import for `cmdline` package.	`pkg/rest/rest.go` `pkg/rest/rest_test.go`
Updated tests to reflect changes in `Endpoint` creation and scheme parsing.	Updated test cases to use `NewEndpoint` instead of `parseRestfulURI`. Adjusted test assertions to match the new `Endpoint` struct and scheme parsing logic.	`pkg/rest/rest_test.go`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

openshift-ci · 2025-03-31T18:35:21Z

Hi @kolyshkin. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sourcery-ai

Hey @kolyshkin - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding comments to the ServiceScheme enum to explain what each value represents.
The error messages in NewEndpoint could be improved to provide more context about the validation failure.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

gbraad · 2025-04-01T06:16:00Z

pkg/rest/rest.go

 		return None, nil
-	case "UNIX":
+	case "unix":


Can a const be used?

Do you mean, a const from the standard packages? I am not aware of any, please suggest.

Or do you mean to introduce and use own constant? If that's the case, I don't think it makes sense here.

I thought they had... https://pkg.go.dev/net/url#pkg-constants "Section is emptry" :-/

gbraad · 2025-04-01T06:17:30Z

pkg/rest/rest.go

 func toRestScheme(s string) (ServiceScheme, error) {
-	switch strings.ToUpper(s) {


This is to ensure no mistakes are made when this is specified manually.
Either use upper or lower

Can you point me out to a case in the code where it is specified manually?

From what I see, it is only used to parse a Scheme field from a structure obtained from url.ParseRequestURI, which ensures Scheme is lowercase, so we can expect the lowercased string here. This was noted in the commit message, too.

I'll let @cfergeau decide as he maintains this codebase.

I just notice that https://github.com/crc-org/vfkit/pull/292/files#diff-e8eb5f08837c85d4c2db17fa463051c4a787efb7cbc9883092f53b24a48514e1R32 uses the input that comes from the request parse; and it assumes the format to be returned always to be lower. Defensive programming is not wrong, as this is not code to be performant or such (and mistakes have been made before). Overall, this looks more maintainable.

Since it is used exclusively to parse a Scheme field from a structure obtained from url.ParseRequestURI, which ensures Scheme is lowercase, we can expect the lowercased string here. Fix the test accordingly. Signed-off-by: Kir Kolyshkin <[email protected]>

It is not used anywhere except in its own unit test. Signed-off-by: Kir Kolyshkin <[email protected]>

gbraad

Haven't tested functionality, but looks cleaner.

openshift-ci · 2025-04-01T07:32:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gbraad
Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cfergeau

3rd commit can be improved in my opinion, the first 2 look good to me.

pkg/rest/rest.go

openshift-ci · 2025-04-09T16:41:23Z

New changes are detected. LGTM label has been removed.

kolyshkin · 2025-04-09T16:42:07Z

3rd commit can be improved in my opinion, the first 2 look good to me.

I have remove the third commit.

cfergeau · 2025-04-10T10:46:46Z

3rd commit can be improved in my opinion, the first 2 look good to me.

I have remove the third commit.

Are you planning to do some work on it? Or should we merge this as is?

cfergeau · 2025-04-10T14:42:27Z

/ok-to-test

openshift-merge-robot · 2025-04-13T08:45:35Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci bot added the do-not-merge/work-in-progress label Mar 31, 2025

openshift-ci bot added the needs-ok-to-test label Mar 31, 2025

kolyshkin marked this pull request as ready for review March 31, 2025 18:36

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 31, 2025

openshift-ci bot requested review from anjannath and praveenkumar March 31, 2025 18:36

sourcery-ai bot reviewed Mar 31, 2025

View reviewed changes

gbraad reviewed Apr 1, 2025

View reviewed changes

kolyshkin added 2 commits March 31, 2025 23:37

pkg/rest: rm validateRestfulURI

1e56123

It is not used anywhere except in its own unit test. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the pkg-rest-refactor branch from f987ceb to c44508a Compare April 1, 2025 06:37

gbraad approved these changes Apr 1, 2025

View reviewed changes

openshift-ci bot assigned gbraad Apr 1, 2025

openshift-ci bot added the lgtm label Apr 1, 2025

cfergeau reviewed Apr 9, 2025

View reviewed changes

pkg/rest/rest.go Outdated Show resolved Hide resolved

kolyshkin force-pushed the pkg-rest-refactor branch from c44508a to 1e56123 Compare April 9, 2025 16:41

openshift-ci bot removed the lgtm label Apr 9, 2025

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Apr 10, 2025

openshift-merge-robot added the needs-rebase label Apr 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/rest: refactor and simplify #292

pkg/rest: refactor and simplify #292

kolyshkin commented Mar 31, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Mar 31, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

openshift-ci bot commented Mar 31, 2025

sourcery-ai bot left a comment

gbraad Apr 1, 2025

kolyshkin Apr 1, 2025

gbraad Apr 1, 2025

gbraad Apr 1, 2025

kolyshkin Apr 1, 2025

gbraad Apr 1, 2025

gbraad left a comment

openshift-ci bot commented Apr 1, 2025

cfergeau left a comment

openshift-ci bot commented Apr 9, 2025

kolyshkin commented Apr 9, 2025

cfergeau commented Apr 10, 2025

cfergeau commented Apr 10, 2025

openshift-merge-robot commented Apr 13, 2025

		func toRestScheme(s string) (ServiceScheme, error) {
		switch strings.ToUpper(s) {

pkg/rest: refactor and simplify #292

Are you sure you want to change the base?

pkg/rest: refactor and simplify #292

Conversation

kolyshkin commented Mar 31, 2025 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Mar 31, 2025 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for Endpoint creation

Updated class diagram for Endpoint

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

openshift-ci bot commented Mar 31, 2025

sourcery-ai bot left a comment

Choose a reason for hiding this comment

gbraad Apr 1, 2025

Choose a reason for hiding this comment

kolyshkin Apr 1, 2025

Choose a reason for hiding this comment

gbraad Apr 1, 2025

Choose a reason for hiding this comment

gbraad Apr 1, 2025

Choose a reason for hiding this comment

kolyshkin Apr 1, 2025

Choose a reason for hiding this comment

gbraad Apr 1, 2025

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 1, 2025

cfergeau left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 9, 2025

kolyshkin commented Apr 9, 2025

cfergeau commented Apr 10, 2025

cfergeau commented Apr 10, 2025

openshift-merge-robot commented Apr 13, 2025

kolyshkin commented Mar 31, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Mar 31, 2025 •

edited

Loading