Card origin_devexp_286 - Added ssh key-based access to private git repository

[soltysh]

@mfojtik @bparees this is the initial and main part of my git card PTAL

[bparees]

more doc about what constitutes a valid secret today?

[soltysh]

I don't think I should explain how to setup a secret over here, it'll only pollute build docs with unnecessary details, which should be covered in secrets. Of course there will be some more information around that topic in our docs, but I don't think it's necessary here.

[bparees]

i'm not looking for major detail, but "secret must be an SSH key for git auth" or something... our API turns out to be our docs frequently.

[soltysh]

Done.

[bparees]

@soltysh is the assumption that they'll use a git url that includes the username for source cloning (ie an ssh url)? you might double check that the web console "create from source" tolerates that format for a source uri.

[soltysh]

@bparees @smarterclayton one question that pop-ed up in my head today, why not using ObjectReference for that? I've taken here the approach @mfojtik did when doing secrets for docker push, but wouldn't it be desirable to use ObjectReference to mark this field as using external object?

[bparees]

ObjectRef would make sense to me..that would enable you to use a secret from a different namespace also.

[smarterclayton]

You're not allowed to reference secrets from other namespaces by design today.

On Apr 28, 2015, at 11:31 AM, Ben Parees notifications@github.com wrote:

In pkg/build/api/types.go:

@@ -111,6 +111,10 @@ type BuildSource struct {
// This allows to have buildable sources in directory other than root of
// repository.
ContextDir string json:"contextDir,omitempty"
+

• // SourceSecretName is the name of a Secret that would be used for setting
• // up the authentication for cloning private repository.
• SourceSecretName string json:"sourceSecretName,omitempty"
  ObjectRef would make sense to me..that would enable you to use a secret from a different namespace also.

—
Reply to this email directly or view it on GitHub.

[soltysh]

Yeah, that's what I was told, and I kinda agree with that argument. Still using an ObjectReference is a good practice IMHO, to notify readers of the API we are reaching somewhere else to get that object, even though withing the scope of your namespace.

[smarterclayton]

Yes, secrets are excluded from that argument for the time being.

On Apr 28, 2015, at 3:43 PM, Maciej Szulik notifications@github.com wrote:

In pkg/build/api/types.go:

@@ -111,6 +111,10 @@ type BuildSource struct {
// This allows to have buildable sources in directory other than root of
// repository.
ContextDir string json:"contextDir,omitempty"
+

• // SourceSecretName is the name of a Secret that would be used for setting
• // up the authentication for cloning private repository.
• SourceSecretName string json:"sourceSecretName,omitempty"
  Yeah, that's what I was told, and I kinda agree with that argument. Still using an ObjectReference is a good practice IMHO, to notify readers of the API we are reaching somewhere else to get that object, even though withing the scope of your namespace.

—
Reply to this email directly or view it on GitHub.

[mfojtik]

LGTM

[0xmichalis]

nit: you can just return script.Close() here.

[smarterclayton]

Hold for review - want to look through this.

[soltysh]

@smarterclayton sure, I'm updating the UI part, as it doesn't allow you to pass git url such as git@github.com:openshift/ruby-hello-world.git yet (thx to @bparees vigilance) and we totally lack tests there, so trying to add those as well.

[soltysh]

@bparees @Kargakis addressed your comments. @smarterclayton ready for final review.
[test] in the mean time.

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2063/)

[soltysh]

Rebased and fixed new-app CLI, which wasn't properly treating git+ssh URLs.

[smarterclayton]

Why does it have to be an SSH key? It could just as easy be basic auth (probably more common) or a client cert.

[soltysh]

I'll just then remove the 2nd sentence.

[soltysh]

Since @bparees insistent on having some description of the contents of the secret, I've reworked that sentence.

[soltysh]

I take that back, per my card we're adding just the SSH key auth method.

[soltysh]

But I'll prepare the mechanism for easily adding the rest of the methods, so in the end that description will be generally for all of the methods.

[soltysh]

@smarterclayton addressed most of your comments, others replied. PTAL.

[mfojtik]

why not use exec.Command here as well? You can then avoid listing files using Go and just "chmod -R 0600 dir"

[soltysh]

It was faster than chmod magic, just to change contents permissions. And passing * is a pain to exec.

[soltysh]

@mfojtik addressed your comments, especially about moving the fixing permissions part to separate method. @smarterclayton PTAL.

[soltysh]

@smarterclayton addressed your comments, except for the two explained inline.

[smarterclayton]

LGTM, [merge]

On May 8, 2015, at 5:38 AM, Maciej Szulik notifications@github.com wrote:

@smarterclayton addressed your comments, except for the two explained inline.

—
Reply to this email directly or view it on GitHub.

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/1858/) (Image: devenv-fedora_1474)

[soltysh]

Rebased.

[openshift-bot]

Evaluated for origin up to a208f1c