cli/connhelper/ssh: add NewSpec utility to prevent parsing URL twice #6034

thaJeztah · 2025-04-23T11:46:40Z

cli/connhelper/ssh: add NewSpec utility

This allows creating a spec from an existing url.URL

cli/connhelper: don't parse URL twice

This function was parsing the same URL twice; first to detect the
scheme, then again (through ssh.ParseURL) to construct a ssh.Spec.

Change the function to use the URL that's parsed, and use ssh.NewSpec
instead.

- How to verify it

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2025-04-23T11:50:38Z

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 58.91%. Comparing base (81a5db6) to head (f105e96).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6034      +/-   ##
==========================================
- Coverage   58.91%   58.91%   -0.01%     
==========================================
  Files         358      358              
  Lines       29983    29988       +5     
==========================================
+ Hits        17664    17666       +2     
- Misses      11339    11341       +2     
- Partials      980      981       +1

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thaJeztah · 2025-04-23T12:22:34Z

Ah, looks like I missed some changes from the branch I created this in;

--- FAIL: TestParseURL (0.00s)
    --- FAIL: TestParseURL/URL_missing_scheme (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: no scheme provided", got "invalid SSH url: no scheme provided"
    --- FAIL: TestParseURL/invalid_URL_with_password (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: plain-text password is not supported", got "invalid SSH url: plain-text password is not supported"
    --- FAIL: TestParseURL/invalid_URL_with_query_parameter (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: query parameters are not allowed: \"foo=bar&bar=baz\"", got "invalid SSH url: query parameters are not allowed: \"foo=bar&bar=baz\""
    --- FAIL: TestParseURL/invalid_URL_with_fragment (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: fragments are not allowed: \"bar\"", got "invalid SSH url: fragments are not allowed: \"bar\""
    --- FAIL: TestParseURL/invalid_URL_without_hostname (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: hostname is empty", got "invalid SSH url: hostname is empty"
    --- FAIL: TestParseURL/#00 (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: hostname is empty", got "invalid SSH url: hostname is empty"
    --- FAIL: TestParseURL/invalid_URL_with_unsupported_scheme (0.00s)
        ssh_test.go:136: assertion failed: expected error "invalid ssh URL: incorrect scheme: https", got "invalid SSH url: incorrect scheme: https"

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This allows creating a spec from an existing url.URL Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This function was parsing the same URL twice; first to detect the scheme, then again (through ssh.ParseURL) to construct a ssh.Spec. Change the function to use the URL that's parsed, and use ssh.NewSpec instead. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

laurazard

LGTM

thaJeztah · 2025-04-23T14:00:00Z

Thx! Let me bring this one in; I have some changes in some other branches that I need to dust off 😂. After that, we may be able to start considering moving this package to a separate module (but we probably should look if everything is in a shape we want it to be in for "separate module").

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Apr 23, 2025

thaJeztah added this to the 28.1.2 milestone Apr 23, 2025

thaJeztah added 3 commits April 23, 2025 14:27

cli/connhelper/ssh: tweak error-message (capitalize SSH)

55073c4

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/connhelper/ssh: add NewSpec utility

11b53da

This allows creating a spec from an existing url.URL Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the connhelper_cleanups_step2 branch from b89f264 to f105e96 Compare April 23, 2025 12:30

laurazard approved these changes Apr 23, 20 8000 25

View reviewed changes

thaJeztah merged commit aadd787 into docker:master Apr 23, 2025
87 checks passed

thaJeztah deleted the connhelper_cleanups_step2 branch April 23, 2025 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli/connhelper/ssh: add NewSpec utility to prevent parsing URL twice #6034

cli/connhelper/ssh: add NewSpec utility to prevent parsing URL twice #6034

cli/connhelper/ssh: add NewSpec utility to prevent parsing URL twice #6034

cli/connhelper/ssh: add NewSpec utility to prevent parsing URL twice #6034

Conversation