Skip to content

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Oct 12, 2023

Fixes #4322 (thanks for the report @alexcb!)

This resolves a regression introduced in #4142. In this previous patch, I'd incorrectly assumed that scp-like URLs can express a subset of "standard"-URLs and so we can always safely convert them for consistency. This isn't true - the URL "git@example.com:foo" should be resolved to the home directory of the host, however, the converted URL "ssh://git@example.com/foo" will be resolved to the root directory.

To resolve this, we need to not perform this conversion. However, we also need preserve the behaviour of firm distinction between SCP and normal URL types (so as to keep proper port parsing).

To do this, we add a new GitURL type to the gitutil package. This new type contains all useful fields shared in common between the standard libraries url package and our custom scp-style url parsing package. This keeps the previous property of a single clean interface to all GitURLs, while also ensuring that we preserve the original URL to pass to the Git CLI (making sure we strip fragments out, which are used as buildkit-level metadata).

As a side-effect of this, the client-side calling code for parsing git urls is simplified (so we don't have to do fragment wrangling at every call point).

(Note: the diff for this seems quite large. This is due to the addition of new tests, as well as the new struct definition, as well as doc-comments on those, instead of massive changes in behavior.)

SCP-style URLs take the following format [1]:

	[user@]server:project.git

These are used and handled by git, so we should have a full-parser for
these, that return a custom URL type.

[1]: https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the fix-ssh-url-modification branch 3 times, most recently from 5f22d19 to bb06eb5 Compare October 12, 2023 14:08
Copy link
Collaborator

@alexcb alexcb left a comment

Choose a reason for hiding this comment

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

thanks for the speedy fix!

@jedevc jedevc force-pushed the fix-ssh-url-modification branch from bb06eb5 to 9a51bb0 Compare October 12, 2023 18:16
This resolves a regression introduced in
50e75e3. In this previous patch, I'd
incorrectly assumed that scp-like URLs can express a subset of
"standard"-URLs and so we can always safely convert them for
consistency. This isn't true - the URL "git@example.com:foo" should be
resolved to the home directory of the host, however, the converted URL
"ssh://git@example.com/foo" will be resolved to the root of the host.

To resolve this, we need to not perform this conversion. However, we
also need preserve the behaviour of firm distinction between SCP and
normal URL types (so as to keep proper port parsing).

To do this, we add a new GitURL type to the gitutil package. This new
type contains all useful fields shared in common between the standard
libraries url package and our custom scp-style url parsing package. This
keeps the previous property of a single clean interface to all GitURLs,
while also ensuring that we preserve the original URL to pass to the Git
CLI (making sure we strip fragments out, which are used as
buildkit-level metadata).

As a side-effect of this, the client-side calling code for parsing
git urls is simplified (so we don't have to do fragment wrangling at
every call point).

Signed-off-by: Justin Chadwell <me@jedevc.com>
@stephanos
Copy link

stephanos commented Mar 15, 2024

Raising this for visibility (hope this is the right PR) - is this the expected behavior? temporalio/docker-builds#187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants