-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Don't depend on github.com/crc-org/vfkit/pkg/rest #25712
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baude PTAL would you have any concerns with that?
I have not tried to look up the history of this but the diff makes we very much in favour, the code seems trivial to maintain on our side.
| return nil, fmt.Errorf("invalid unix uri: socket path length exceeds macOS limits") | ||
| } | ||
| return []string{"--restful-uri", fmt.Sprintf("unix://%s", uri.Path)}, nil | ||
| case "TCP", "HTTP": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK podman always seem to use an http API socket so this could be simplified even further.
(I might need to check why we use a tcp socket at all, unix seems much better suited for local IPC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see things like mc.AppleHypervisor.Vfkit.Endpoint = localhostURI + ":" + strconv.Itoa(randPort), but that’s a few layers away from the call site.
I’m all in favor of simplifying further, but I’ll leave that to Podman experts.
|
Note that |
|
… and another view is that this removes 21.75 % of total lines in |
Via github.com/gin-gonic/gin , this depends on _several_ large encoding / decoding packages, including a JIT compiler. Maintaining <60 lines of code ourselves seems well worth it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
wfm |
|
@Luap99 I think I’ll leave this as a prototype for now, and I’ll let you finish / replace this as part of the larger size-reducing work. |
Sure, works for me. |
|
Note to self: #25720 is a superset, this should be closed after that merges. |
|
Thanks @Luap99 ! |
Warning: Completely untested, and it would have been better authored as a commit just copying the code, + a set of refactors. I’ll happily close this, and leave full credit to, anyone who wants to do this properly.
Via
github.com/gin-gonic/gin, this depends on several large encoding / decoding packages, including a JIT compiler. Maintaining <60 lines of code ourselves seems well worth it.Note that this dependency chain only exists on
darwin.Impact:
927 files changed, 103 insertions(+), 505542 deletions(-)dropping
Cc: @Luap99
Does this PR introduce a user-facing change?