Git spec handling revamp for 3.7

[jim-minter]

improve git repository spec parsing and hopefully avoid hitting issues due to tightened URL parsing in go 1.8.

fix #14831
fix #15005

[jim-minter]

@coreydaley
@openshift/devex
@bparees not sure whether a conversation is warranted about if this goes in 3.6.0 or not

[test][testextended][extended:core(builds)]

[jim-minter]

@coreydaley apologies, this was a resurrection of a patch I already had; didn't realise that the above issues were in your name before I sent the PR

[coreydaley]

@jim-minter Well I am certainly not going to complain about you fixing them ;)

[gabemontero]

Is there similar logic now in the s2i code base as well (admissions, I could search, but am technically on PTO :-) )? Not that I had a lot of pride in the name MungeNoProtocolURL :-) , but part of the original motivation for the structure of this change was to have consistent URL parsing between origin and s2i, where we pushed as much logic into s2i as possible. Having this logic written one time and shared in both repos still seems valuable, no?

[gabemontero]

To clarify, given the associated removal in the s2i repo, I'm just curious as to why it was no longer getting called there as well (but it has been a while ... perhaps I'm forgetting a reason why it was not needed there).

[coreydaley]

@gabemontero I've got this issue open (openshift/source-to-image#206) to do some refactoring/sharing of code between origin and source-to-image, which maybe should be expanded to cover as much as possible?

[gabemontero]

@coreydaley yeah if we are not going to address placing this fix somehow into s2i via openshift/source-to-image#771, then either doing so subsequently in a common fashion, or determining/documenting why it is not needed, seems prudent to me.

[bparees]

it looks like Munge* was not being called at all in s2i, i think we must have refactored around it at some point and not removed it. So the removal isn't a concern, but the fact that s2i itself still uses url.Parse in ways that i think will break, is.

[gabemontero]

100% agreed. Having a single code path shared by both scenarios that addresses the multitude of pain points wrt URL parsing in golang should be the ultimate goal, and what I wanted (but may not have entirely succeeded) to convey. I'll defer on whether it is done in one fell swoop or over a series of changes.

[bparees]

this is a step in the right direction, but the build logic itself still has other git-url managing logic (shared with s2i, as @gabemontero alluded to, though neither invoke the munge function anymore).

Specifically this path will need to be cleaned up as well since it ultimately relies on url.Parse:
https://github.com/openshift/origin/blob/master/pkg/build/builder/source.go#L170
https://github.com/openshift/source-to-image/blob/master/pkg/scm/git/git.go#L104-L131
https://github.com/openshift/source-to-image/blob/master/pkg/scm/git/git.go#L187-L209
https://github.com/openshift/source-to-image/blob/master/pkg/scm/git/git.go#L306-L373

[bparees]

hm, maybe that's incorrect since ParseRepository will be called before any of that logic, thus ensuring a clean url reaches that code path.

however s2i itself (cli invocation) will still be broken, i think.

[bparees]

actually i was right the first time. this fixes the ParseRepository invocation called at one point in the build path, but the fetchSource logic that actually clones the repo does not go through ParseRepository today, so it will still break there.

[jim-minter]

@bparees I don't follow you - I think that url.Parse is only called in the above code path after the ssh case has been ruled out, therefore the code is "OK".

I do agree that s2i and origin appear to require similar logic here and that given the practice is to vendor in to origin, this new code is probably in the wrong place.

I guess I was aiming at taking a single small step in order to enable Go 1.8. In principle I think some code like this should replace ParseFile, ParseSSH, ParseURL, but because of their side-effects it's a more involved change. Let me take another look.

[jim-minter]

ah, there's a bug with relative file paths too, hold on.

[bparees]

@bparees I don't follow you - I think that url.Parse is only called in the above code path after the ssh case has been ruled out, therefore the code is "OK".

ah, ok yes that's true. it still looks dangerous and i'd like to see us get to a more centralized flow like what you're doing here, for anything that's processing a git spec.

I guess I was aiming at taking a single small step in order to enable Go 1.8. In principle I think some code like this should replace ParseFile, ParseSSH, ParseURL, but because of their side-effects it's a more involved change. Let me take another look.

Fair enough, but we've got (some) time since we're not going to try to jam this in 3.6.0, so i think it's worth seeing if we can get this all refactored at once (Assuming, after your investigation, there is still refactoring to do..but i think there is, because we are manipulating the raw git spec value in two different codepaths currently)

[jim-minter]

Agreed.

[liggitt]

cc @deads2k

[jim-minter]

@bparees ready for review - need to update s2i commit SHA and squash before release, otherwise it's passing tests

[bparees]

@jim-minter lgtm, so overall the dance that needs to happen here:

1. get the s2i changes into both master and 3.6 branches of the s2i repo
2. update this PR w/ the s2i master bump commit, this PR will merge after the 3.6 branch is cut (so it's gonna be a bit, unfortunately, but discussions have started about branching).
3. create another PR against the 3.6 branch (once it exists) that includes these changes and a bump commit from the s2i 3.6 branch, so that we can get this into 3.6.1 (but not 3.6.0).

@deads2k will then want to pull in 2 or 3 for his rebase effort, depending on the target of that rebase.

[jim-minter]

Thanks @bparees . Steps 1 and 2 done (2 assuming that openshift/source-to-image#771 is the next thing to be committed to s2i master).

[jim-minter]

@deads2k I believe you should be able to cherry pick these for the rebase.

[deads2k]

Wow, no conflicts with the rebase. It was meant to be.

[bparees]

I'll be the annoying one and ask: do we need this code in s2i as well in case proxy url's are supplied there?

The standalone s2i does not handle proxies or accept a proxy url. It assumes you've setup your git environment properly so that cloning is possible directly.

[bparees]

lgtm pending passing tests. @deads2k once tests pass you'll want to grab the new commit into your rebase PR and then the build extended tests should pass for you.

[gabemontero]

On Tue, Jul 18, 2017 at 10:21 AM, Ben Parees ***@***.***> wrote: I'll be the annoying one and ask: do we need this code in s2i as well in case proxy url's are supplied there? The standalone s2i does not handle proxies or accept a proxy url. It assumes you've setup your git environment properly so that cloning is possible directly.

ok cool thx for the clarification.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15071 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADbadD0pNWKur1OqLC1gnXr85Vb0Z4DUks5sPL6rgaJpZM4OPcd2> .

[deads2k]

lgtm pending passing tests. @deads2k once tests pass you'll want to grab the new commit into your rebase PR and then the build extended tests should pass for you.

Is there a specific commit to pick? If I have to rebase the entire thing, probably post-rebase.

[jim-minter]

@deads2k assuming it passes tests, daea5d6 on this PR is the additional commit.

[deads2k]

This didn't test green, so I did not pick it into #15234

[bparees]

looks like it needs bindata generated.

[jim-minter]

bindata updated. Most of this PR is now in via being cherry-picked; what's left is a missed Godeps update from the cherry pick and the fixes to proxy url parsing

[bparees]

[merge][severity:blocker]

[bparees]

[merge]

[bparees]

not sure why this is claiming a merge conflict, Github doesn't think it is...but @jim-minter please rebase when you get a chance.

      "Auto-merging pkg/build/util/util.go", 
        "CONFLICT (content): Merge conflict in pkg/build/util/util.go", 
        "Auto-merging pkg/build/apis/build/validation/validation.go", 
        "Auto-merging Godeps/Godeps.json", 
        "Automatic merge failed; fix conflicts and then commit the result."
    ], 

[bparees]

ah, now the conflict is showing up.

[bparees]

@jim-minter bump for rebase

[jim-minter]

@bparees pushed, thanks

[openshift-bot]

Evaluated for origin testextended up to 4b5e537

[openshift-bot]

Evaluated for origin test up to 4b5e537

[bparees]

[merge]

[openshift-bot]

Evaluated for origin merge up to 4b5e537

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3387/) (Base Commit: 8833a3a) (PR Branch Commit: 4b5e537)

[openshift-bot]

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/896/) (Base Commit: 8833a3a) (PR Branch Commit: 4b5e537) (Extended Tests: core(builds))

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1356/) (Base Commit: 0140c2d) (PR Branch Commit: 4b5e537) (Extended Tests: blocker) (Image: devenv-rhel7_6477)