Optional Git URL rewrite rule #908

MayukhSobo · 2021-10-20T05:22:18Z

Changes

Is your feature request related to a problem? Please describe.

Based on user feedback, there could be a scenario where someone defines a Git repository as the build source that contains a submodule that uses a different Git URL style. There are two known styles:

HTTPS style, e.g. https://github.com/shipwright-io/build.git
Git+SSH style, e.g. git@github.com:shipwright-io/build.git

For the HTTPS style, we assume users configure publicly available repositories that do not require any authorization. For the sake of completeness, the HTTPS URLs can be used with deploy key credentials, but we do not openly advertise this options in the docs. The Git+SSH style must come with credentials that contain SSH keys, obviously.

Describe the solution you'd like

When working with GitHub Enterprise (or any other private Git instance for that matter), developers often implement a rewrite rule to use the somewhat more readable HTTPS URLs while Git is working via SSH under the cover. For this, users run the following command once:

git config --global url.ssh://git@github.ibm.com/.insteadOf https://github.ibm.com/

This makes for a smoother working as you do not have to check the style each time, however, it covers a bit of the actual things that happen. Now, we could introduce the same "magic" in the Git step used by Shipwright build. However, this needs to be generic, as we work with different Git services, like GitHub, or GitLab. Also, we should not make that the default behavior right from the start, so I would suggest to make it configurable via a command line flag (--git-url-rewrite-rule).

The flag would be set by our controller setting up the Git step. And the controller would look-up the setting by checking an environment variable (since we have no config map for this).

Describe alternatives you've considered
Provide documentation about this pit-fall and reference it whenever someone runs into it.

Additional context

References:

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

…r and pass it in the git module

MayukhSobo · 2021-10-20T09:30:20Z

/assign @gabemontero

gabemontero · 2021-10-20T12:25:11Z

@MayukhSobo can you please update the description so that a release note is specified

MayukhSobo · 2021-10-22T06:35:15Z

@gabemontero Can you please elaborate what exactly you meant?

SaschaSchwarze0

Please extend https://github.com/shipwright-io/build/blob/main/docs/configuration.md.

SaschaSchwarze0 · 2021-10-25T12:14:46Z

cmd/git/main.go

@@ -313,6 +320,13 @@ func clone(ctx context.Context) error {
 			if _, err := git(ctx, "config", "--global", "credential.helper", fmt.Sprintf("store --file %s", credHelperFile.Name())); err != nil {
 				return err
 			}
+			if flagValues.gitURLRewrite {
+				sshURL := convertToSSH(repoURL.Host)
+				if _, err := git(ctx, "config", "--global", sshURL); err != nil {


Does that work? In examples such as https://stackoverflow.com/questions/1722807/how-to-convert-git-urls-to-http-urls, there are always two arguments ("url.https://github.com/.insteadOf" and "git://github.com/"). You're passing in the sshURL variable that contains both as one argument.

@SaschaSchwarze0 @HeavyWombat Well I haven't tested it but I am sure it would work. If you would take a careful look into the method/function convertToSSH, you would notice that there is a space between url.ssh://git@%s/.insteadOf and https://%s/". This ensures that you are passing two space-separated commands. So unless the git(ctx, "config", "--global", sshURL); works differently, this would work well.

Also, the reason to merge this into one function is that for such a trivial task, I felt 2 function calls or polluting the local namespace would be costlier in the future.

@MayukhSobo you can be sure that I take a careful look at code that I am reviewing. And I obviously saw the space. Nevertheless, you are running this command:

git config --global "url.ssh://git@github.com/.insteadOf https://github.com/"

while this is probably the correct one:

git config --global url.ssh://git@github.com/.insteadOf https://github.com/

But once you test this, I am sure you notice that on your own.

gabemontero · 2021-10-25T14:19:39Z

@gabemontero Can you please elaborate what exactly you meant?

the release note section in your PR's description here should have some text following the Release Note heading.

See @SaschaSchwarze0 's PR #909 (comment) for example.

Your change here seems like it should have release not text.

From what I see from examining your description, you did not make any changes from what our PR template provided. Try editing the description, reading the instructions provided by the PR template, and see if you can get a properly formatted release note.

If you have problem interpreting the instructions, let us know here in the PR and we'll go from there.

HeavyWombat

First of all, thanks for your efforts here. I left a comment and I agree with @SaschaSchwarze0, it does not look correct how the config command looks like. Left a suggestion as a starting point. Also, please have a look into options on how to create a unit test for this feature. Lastly, like @gabemontero already pointed out, we need a release notes section. Have a look at previous PRs to get a feeling.

HeavyWombat · 2021-10-25T16:03:22Z

cmd/git/main.go

@@ -52,6 +52,7 @@ type settings struct {
 	resultFileCommitAuthor string
 	secretPath             string
 	skipValidation         bool
+	gitURLRewrite 		   bool


Nit: There seems to be a mix up of spaces versus tabs. Have a look whether you can unify that.

Yes, I saw that previously. Not sure how to fix that. It seems like I am using Goland IDE which is causing it. I would try to fix it but don't you think go fmt should have auto-corrected it?

HeavyWombat · 2021-10-25T16:12:30Z

cmd/git/main.go

+			if flagValues.gitURLRewrite {
+				sshURL := convertToSSH(repoURL.Host)
+				if _, err := git(ctx, "config", "--global", sshURL); err != nil {
+					return err
+				}
+			}
+


I agree with @SaschaSchwarze0, the Git config command seems incomplete and is also nested under the wrong if branch. This logic should be independent of the input type. With that being said, I hacked together a suggestion that is extracting the host from the flagValues.url. It then uses the host name to create the Git config command. Maybe you can use this as a start to continue to work.

Suggested change

if flagValues.gitURLRewrite {

sshURL := convertToSSH(repoURL.Host)

if _, err := git(ctx, "config", "--global", sshURL); err != nil {

return err

}

}

}

if flagValues.gitURLRewrite {

var hostname string

switch {

case strings.HasPrefix(flagValues.url, "git@"):

trimmed := strings.TrimPrefix(flagValues.url, "git@")

splitted := strings.SplitN(trimmed, ":", 2)

hostname = splitted[0]

case strings.HasPrefix(flagValues.url, "http"):

repoURL, err := url.Parse(flagValues.url)

if err != nil {

return err

}

hostname = repoURL.Host

default:

return fmt.Errorf("unknown URL type: %q", flagValues.url)

}

_, err := git(ctx,

"config",

"--global",

fmt.Sprintf("url.ssh://git@%s/.insteadOf", hostname),

fmt.Sprintf("https://%s/", hostname))

if err != nil {

return err

}

}

HeavyWombat · 2021-10-25T16:13:48Z

deploy/500-controller.yaml

@@ -34,6 +34,8 @@ spec:
              value: "shipwright-build"
            - name: GIT_CONTAINER_IMAGE
              value: ko://github.com/shipwright-io/build/cmd/git
+            - name: GIT_REWRITE_RULE
+              value: 'true'


Nit: Keeping the "style" of the other string.

Suggested change

value: 'true'

value: "true"

openshift-ci · 2021-10-26T06:23:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from gabemontero after the PR has been reviewed.

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

HeavyWombat · 2021-11-04T19:38:15Z

Superseded by #937

Updated the env configs for the rewrite rule

821b04b

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Label for when a PR has specified a release note labels Oct 20, 2021

openshift-ci bot requested review from adambkaplan and HeavyWombat October 20, 2021 05:22

MayukhSobo changed the title ~~Updated the env configs for the rewrite rule~~ Introduce optional Git URL rewrite rule Oct 20, 2021

MayukhSobo changed the title ~~Introduce optional Git URL rewrite rule~~ Optional Git URL rewrite rule Oct 20, 2021

MayukhSobo added 2 commits October 20, 2021 12:43

Added a method to optionally take the rewrite rule from the controlle…

6541517

…r and pass it in the git module

Some code refactoring

076ed51

openshift-ci bot assigned gabemontero Oct 20, 2021

MayukhSobo marked this pull request as ready for review October 20, 2021 09:30

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2021

SaschaSchwarze0 requested changes Oct 25, 2021

View reviewed changes

HeavyWombat requested changes Oct 25, 2021

View reviewed changes

Merge branch 'shipwright-io:main' into feature/github_rewrite_rule

840fac1

MayukhSobo requested a review from SaschaSchwarze0 October 26, 2021 06:37

HeavyWombat closed this Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional Git URL rewrite rule #908

Optional Git URL rewrite rule #908

MayukhSobo commented Oct 20, 2021 •

edited

Loading

MayukhSobo commented Oct 20, 2021

gabemontero commented Oct 20, 2021

MayukhSobo commented Oct 22, 2021

SaschaSchwarze0 left a comment

SaschaSchwarze0 Oct 25, 2021

MayukhSobo Oct 26, 2021

MayukhSobo Oct 26, 2021

SaschaSchwarze0 Oct 26, 2021

gabemontero commented Oct 25, 2021

HeavyWombat left a comment

HeavyWombat Oct 25, 2021

MayukhSobo Oct 26, 2021

HeavyWombat Oct 25, 2021

HeavyWombat Oct 25, 2021

openshift-ci bot commented Oct 26, 2021

HeavyWombat commented Nov 4, 2021

-			if flagValues.gitURLRewrite {
-				sshURL := convertToSSH(repoURL.Host)
-				if _, err := git(ctx, "config", "--global", sshURL); err != nil {
-					return err
-				}
-			}
+	}
+	if flagValues.gitURLRewrite {
+		var hostname string
+		switch {
+		case strings.HasPrefix(flagValues.url, "git@"):
+			trimmed := strings.TrimPrefix(flagValues.url, "git@")
+			splitted := strings.SplitN(trimmed, ":", 2)
+			hostname = splitted[0]
+		case strings.HasPrefix(flagValues.url, "http"):
+			repoURL, err := url.Parse(flagValues.url)
+			if err != nil {
+				return err
+			}
+			hostname = repoURL.Host
+		default:
+			return fmt.Errorf("unknown URL type: %q", flagValues.url)
+		}
+		_, err := git(ctx,
+			"config",
+			"--global",
+			fmt.Sprintf("url.ssh://git@%s/.insteadOf", hostname),
+			fmt.Sprintf("https://%s/", hostname))
+		if err != nil {
+			return err
+		}
+	}

Optional Git URL rewrite rule #908

Optional Git URL rewrite rule #908

Conversation

MayukhSobo commented Oct 20, 2021 • edited Loading

Changes

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Submitter Checklist

Release Notes

MayukhSobo commented Oct 20, 2021

gabemontero commented Oct 20, 2021

MayukhSobo commented Oct 22, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Oct 25, 2021

HeavyWombat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 26, 2021

HeavyWombat commented Nov 4, 2021

MayukhSobo commented Oct 20, 2021 •

edited

Loading