Introduce Git config URL rewrite option #937

HeavyWombat · 2021-11-04T19:37:09Z

Changes

There is use case that pops up more often than others and this is that a user wants to build a private repository that contains a submodule, which itself is also a private repository. Sometimes, the submodule is configured using a HTTPS URL while the main repository uses Git+SSH. This leads to errors in the Git step, because the private submodule cannot be loaded due to missing access credentials. Obviously, the HTTPS URL does not accept the Git private key that is configured. The typical local solution is to setup an insteadOf URL rewrite rule in the Git config to dynamically translate the URL types. This PR brings the same feature to the Git step. It is disabled by default.

Add Git wrapper command line flag to enable setting up Git config URL rewrite.
Add environment variable GIT_ENABLE_REWRITE_RULE to enable the feature for
the step in the build strategy.
Add code to set the Git wrapper CLI flag if configured.

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

Git step now supports setting an optional rewrite rule so that HTTPS URLs are translated into Git+SSH URLs during the Git clone and Git submodule operations.

SaschaSchwarze0 · 2021-11-05T07:15:19Z

cmd/git/main.go

+			hostname = repoURL.Host
+
+		default:
+			return fmt.Errorf("unknown URL type: %q", flagValues.url)


Not sure if this is a good idea. If you check https://stackoverflow.com/questions/31801271/what-are-the-supported-git-url-formats, then ssh:// and git:// (I think GitHub just stopped supporting it) would also be possible. We could add more cases. Anyway, imo we should just print a warning that we cannot parse it and therefore do not configure a rewrite.

GitHub did in fact stop supporting (introducing "brown outs") for git://

Changed implementation to print a message in case it is an unknown or unsupported type.

SaschaSchwarze0 · 2021-11-05T07:19:49Z

pkg/config/config.go

@@ -68,6 +68,9 @@ const (

 	terminationLogPathDefault = "/dev/termination-log"
 	terminationLogPathEnvVar  = "TERMINATION_LOG_PATH"
+
+	// environment variable for the Git rewrite setting
+	useGitRewriteRule = "ENABLE_GIT_REWRITE_RULE"


Please document the new environment variable in https://github.com/shipwright-io/build/blob/main/docs/configuration.md. For consistency, I would also move GIT_ to the beginning of the environment variable name.

gabemontero

I'm not calling out that this PR has to take on the command line flag item I noted below

But the situation just grows ...

gabemontero · 2021-11-05T12:43:57Z

cmd/git/main.go

+			hostname = repoURL.Host
+
+		default:
+			return fmt.Errorf("unknown URL type: %q", flagValues.url)


GitHub did in fact stop supporting (introducing "brown outs") for git://

gabemontero · 2021-11-05T12:44:44Z

cmd/git/main.go

@@ -82,6 +83,7 @@ func init() {

 	// Mostly internal flag
 	pflag.BoolVar(&flagValues.skipValidation, "skip-validation", false, "skip pre-requisite validation")
+	pflag.BoolVar(&flagValues.gitURLRewrite, "git-url-rewrite", false, "set Git config to use url-insteadOf setting based on Git repository URL")


hmm ... when are we going to "bite the bullet" and start controlling behavior like this via settings in config maps vs. start up flags

I know, I know. Do not have a good answer for that. With a config map, I think there was the discussion that this will lead to higher expectations, for example that an operator will likely assume that a change in the config map will immediately be used by the controller.

Imo the command line flags in step containers like git or mutate-image will never go away because the BuildRun pod in the user's namespace will never access a ConfigMap that represents the Shipwright configuration (in the shipwright namespace). The other question is when the environment variables on the controller yaml will get replaced by a ConfigMap. Imo this can happen at any time, though as long as it has not been done, we should not stop any feature development that by coincident needs a configuration.

a config map with the config can be created in the user's namespace by the shipwright controller, based on config (including config maps) that drive the behavior of the controller, including how it sets up the build run pod, so that the user namespace config map drives the behavior of the build run pod

such a pattern is employed in openshift builds today

as our discussion here reflects, a separate effort, including most likely an EP, should be pursued ... probably discussion in community or grooming meetings to collectively decide when we will get to this

@HeavyWombat efforts here just remind us we keep putting this off :-)

@gabemontero The config map idea has an issue as far as I know. We just need someone to find time and the energy to do it. Independent of that, are you good to let this PR go into the main branch? Thanks.

@HeavyWombat yeah the config map idea has been discussed, but I could not find a tracking issue (via https://github.com/shipwright-io/build/issues?q=is%3Aissue+is%3Aopen+config+in%3Atitle)

but we don't need to sort that out here

so regardless , yes, as I said before, I'm fine with the PR moving forward independent of that.

that said, non-shipwright items have popped for me and unfortunately it will be at least until second half of next week before I can do a legitimate code review (vs. the cursory scan I did last week)

if you don't want to wait that long of course assign anybody from the @sbose78 @adambkaplan @imjasonh @coreydaley @otaviof list of hatters and they can pick up

thanks

Add Git wrapper command line flag to enable setting up Git config URL rewrite. Add environment variable `GIT_ENABLE_REWRITE_RULE` to enable the feature for the step in the build strategy. Add code to set the Git wrapper CLI flag if configured. Signed-off-by: Mayukh Sarkar <mayukh.sarkar1@ibm.com>

SaschaSchwarze0

/approve

openshift-ci · 2021-11-05T13:23:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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:

~~OWNERS~~ [SaschaSchwarze0]

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

SaschaSchwarze0 · 2021-11-25T07:28:18Z

/lgtm

We just had the next customer who used an https URL with an SSH secret. I'd like to take away their frustration finally.

openshift-ci bot added the release-note Label for when a PR has specified a release note label Nov 4, 2021

openshift-ci bot requested review from adambkaplan and SaschaSchwarze0 November 4, 2021 19:37

HeavyWombat mentioned this pull request Nov 4, 2021

Optional Git URL rewrite rule #908

Closed

4 tasks

HeavyWombat requested a review from gabemontero November 4, 2021 19:38

HeavyWombat force-pushed the add/git-rewrite-rule branch from 8c92931 to 19104f9 Compare November 4, 2021 19:39

SaschaSchwarze0 requested changes Nov 5, 2021

View reviewed changes

SaschaSchwarze0 added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 5, 2021

gabemontero reviewed Nov 5, 2021

View reviewed changes

HeavyWombat force-pushed the add/git-rewrite-rule branch from 19104f9 to f570d46 Compare November 5, 2021 13:18

HeavyWombat requested review from gabemontero and SaschaSchwarze0 November 5, 2021 13:20

SaschaSchwarze0 approved these changes Nov 5, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021

HeavyWombat requested review from coreydaley and sbose78 November 12, 2021 17:02

openshift-ci bot assigned SaschaSchwarze0 Nov 25, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2021

openshift-merge-robot merged commit d0f5e9b into main Nov 25, 2021

SaschaSchwarze0 deleted the add/git-rewrite-rule branch November 25, 2021 07:31

SaschaSchwarze0 added this to the release-v0.7.0 milestone Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Git config URL rewrite option #937

Introduce Git config URL rewrite option #937

HeavyWombat commented Nov 4, 2021 •

edited

Loading

SaschaSchwarze0 Nov 5, 2021

gabemontero Nov 5, 2021

HeavyWombat Nov 5, 2021

SaschaSchwarze0 Nov 5, 2021

HeavyWombat Nov 5, 2021

gabemontero left a comment

gabemontero Nov 5, 2021

gabemontero Nov 5, 2021

HeavyWombat Nov 5, 2021

SaschaSchwarze0 Nov 5, 2021

gabemontero Nov 8, 2021

HeavyWombat Nov 11, 2021

gabemontero Nov 12, 2021

SaschaSchwarze0 left a comment

openshift-ci bot commented Nov 5, 2021

SaschaSchwarze0 commented Nov 25, 2021

Introduce Git config URL rewrite option #937

Introduce Git config URL rewrite option #937

Conversation

HeavyWombat commented Nov 4, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

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 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 5, 2021

SaschaSchwarze0 commented Nov 25, 2021

HeavyWombat commented Nov 4, 2021 •

edited

Loading