code review 106740044: go get: Support for IBM DevOps Services (hub.jazz.net) ...

go get: Support for IBM DevOps Services (hub.jazz.net) git repos

[sirnewton_01, 2014-06-11 00:00:39]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-06-11 04:07:11]

This could use some tests.
 On Jun 10, 2014 9:03 PM, <newton688@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> go get: Support for IBM DevOps Services (hub.jazz.net) git repos
>
> Please review this at https://codereview.appspot.com/106740044/
>
> Affected files (+9, -0 lines):
>   M src/cmd/go/vcs.go
>
>
> Index: src/cmd/go/vcs.go
> ===================================================================
> --- a/src/cmd/go/vcs.go
> +++ b/src/cmd/go/vcs.go
> @@ -613,6 +613,15 @@
>                 check:  launchpadVCS,
>         },
>
> +       // IBM DevOps Services (JazzHub)
> +       {
> +               prefix: "hub.jazz.net/git",
> +               re:     `^(?P<root>hub.jazz.net/git/[A-Za-z0-9_
> <http://hub.jazz.net/git/%5BA-Za-z0-9_>.\-]+/[A-Za-z0-9_.\-
> ]+)(/[A-Za-z0-9_.\-]+)*$`,
> +               vcs:    "git",
> +               repo:   "https://{root}",
> +               check:  noVCSSuffix,
> +       },
> +
>         // General syntax for any server.
>         {
>                 re:   `^(?P<root>(?P<repo>([a-z0-9.\
> -]+\.)+[a-z0-9.\-]+(:[0-9]+)?/[A-Za-z0-9_.\-/]*?)\.(?P<vcs>
> bzr|git|hg|svn))(/[A-Za-z0-9_.\-]+)*$`,
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[sirnewton_01, 2014-06-15 18:14:30]

Thank you for having a look.

I'm looking at writing tests. Are there test suites for github and the other
sites. I hope to write something similar.

On 2014/06/11 04:07:11, bradfitz wrote:
> This could use some tests.

[bradfitz, 2014-06-15 19:13:55]

I guess it's in the go.tools sub-repo now.

See code.google.com/p/go.tools's go/vcs/vcs_test.go

I'm not sure which is considered upstream these days. Chris, Andrew?



On Sun, Jun 15, 2014 at 11:14 AM, <newton688@gmail.com> wrote:

> Thank you for having a look.
>
> I'm looking at writing tests. Are there test suites for github and the
> other sites. I hope to write something similar.
>
> On 2014/06/11 04:07:11, bradfitz wrote:
>
>> This could use some tests.
>>
>
>
> https://codereview.appspot.com/106740044/
>

[sirnewton_01, 2014-06-16 00:59:59]

Thanks, I have added a table-driven unit test to test out the regexp.

On 2014/06/15 19:13:55, bradfitz wrote:
> I guess it's in the go.tools sub-repo now.
> 
> See code.google.com/p/go.tools's go/vcs/vcs_test.go
> 
> I'm not sure which is considered upstream these days. Chris, Andrew?
>

[adg, 2014-06-16 01:18:36]

On 16 June 2014 05:13, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> I'm not sure which is considered upstream these days. Chris, Andrew?


They're supposed to be kept in sync, but I don't think we defined upstream,
per se. It's possible they could be considered forked, now. Changes that
affect cmd/go should be made to the cmd/go sources.

Andrew

[sirnewton_01, 2014-06-18 01:43:34]

> 
> They're supposed to be kept in sync, but I don't think we defined upstream,
> per se. It's possible they could be considered forked, now. Changes that
> affect cmd/go should be made to the cmd/go sources.
> 

Should I provide a patch to the go.tool version of cmd/go? Do the same
contribution guidelines apply for that repo?

Do the changes look okay at this point? I have attached a set of unit tests.

Thanks,
Chris

[bradfitz, 2014-06-18 16:34:59]

https://codereview.appspot.com/106740044/diff/60001/src/cmd/go/vcs_test.go
File src/cmd/go/vcs_test.go (right):

https://codereview.appspot.com/106740044/diff/60001/src/cmd/go/vcs_test.go#ne...
src/cmd/go/vcs_test.go:13: type dosTest struct {
not sure why this has to be dosTest and not generic to all types of import
paths, like the tests in go.tools.  Could we mirror those tests here, or at
least its structure, and just add jazz.net test entries?

[sirnewton_01, 2014-06-19 01:47:16]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org, adg@golang.org
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[sirnewton_01, 2014-06-20 01:23:13]

On 2014/06/18 16:34:59, bradfitz wrote:
>
https://codereview.appspot.com/106740044/diff/60001/src/cmd/go/vcs_test.go#ne...
> src/cmd/go/vcs_test.go:13: type dosTest struct {
> not sure why this has to be dosTest and not generic to all types of import
> paths, like the tests in go.tools.  Could we mirror those tests here, or at
> least its structure, and just add http://jazz.net test entries?

I ported the vcs_test.go structure from go.tools and moved the dosTests into
that structure.

Is this what you had in mind?

Thanks,
Chris

[bradfitz, 2014-06-24 00:34:19]

R=adg

Leaving for Andrew, mostly as a policy decision on how many more code hosting
sites we want to support.  Was the line ever drawn at 4, or is it open season to
include any?

[sirnewton_01, 2014-07-08 14:18:43]

Hi,

Just wondering what the status is on this patch. Is there anything I can do to
fix it up?

Thanks,
Chris

[minux, 2014-07-08 16:47:11]

gentle ping, adg.

On 2014/07/08 14:18:43, sirnewton_01 wrote:
> Just wondering what the status is on this patch. Is there anything I can do to
> fix it up?
We're waiting for Andrew to make the decision.
(Andrew was on vacation.)

[adg, 2014-07-09 03:14:18]

LGTM

[adg, 2014-07-09 03:16:30]

Could you please sign the CLA?

http://golang.org/doc/contribute.html#copyright

[adg, 2014-07-09 03:20:31]

Oh, and could you please also update the documentation in vcs.go to include an
example of hub.jazz.net and run mkdoc.sh ?

See: http://golang.org/cmd/go/#hdr-Remote_import_paths

[sirnewton_01, 2014-07-13 02:03:44]

On 2014/07/09 03:20:31, adg wrote:
> Oh, and could you please also update the documentation in vcs.go to include an
> example of http://hub.jazz.net and run mkdoc.sh ?
> 
> See: http://golang.org/cmd/go/#hdr-Remote_import_paths

I have signed the CLA, added an example to help.go and generated a new doc.go
with the mkdoc.sh script.

Can you PTAL?

Thank you,
Chris

[adg, 2014-07-15 01:27:12]

*** Submitted as https://code.google.com/p/go/source/detail?r=9bf5f4dd2857 ***

go get: Support for IBM DevOps Services (hub.jazz.net) git repos

LGTM=adg
R=golang-codereviews, adg, minux
CC=golang-codereviews
https://codereview.appspot.com/106740044

Committer: Andrew Gerrand <adg@golang.org>

[gobot, 2014-07-15 01:36:44]

This CL appears to have broken the android-arm-crawshaw builder.
See http://build.golang.org/log/cc2dbc7029a8bb50fbae517633f05bcdc6b430dd

[minux, 2014-07-15 01:40:46]

On Mon, Jul 14, 2014 at 9:36 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the android-arm-crawshaw builder.
> See http://build.golang.org/log/cc2dbc7029a8bb50fbae517633f05bcdc6b430dd

A real failure for android port.

--- FAIL: TestRepoRootForImportPath (0.65s)
vcs_test.go:109: RepoRootForImport("code.google.com/p/go"): Get
https://code.google.com/p/go/source/checkout?repo=: x509: failed to load
system roots and no roots provided
FAIL

we need some special handling for crypto/x509 on android.

[dave_cheney.net, 2014-07-15 01:55:07]

Yup, and it broke all the nacl builders as well, I guess we fake
/etc/hosts for the hosts that we expect, and we didn't expect this
one.

On Tue, Jul 15, 2014 at 11:40 AM, minux <minux@golang.org> wrote:
>
> On Mon, Jul 14, 2014 at 9:36 PM, <gobot@golang.org> wrote:
>>
>> This CL appears to have broken the android-arm-crawshaw builder.
>> See http://build.golang.org/log/cc2dbc7029a8bb50fbae517633f05bcdc6b430dd
>
> A real failure for android port.
>
> --- FAIL: TestRepoRootForImportPath (0.65s)
> vcs_test.go:109: RepoRootForImport("code.google.com/p/go"): Get
> https://code.google.com/p/go/source/checkout?repo=: x509: failed to load
> system roots and no roots provided
> FAIL
>
> we need some special handling for crypto/x509 on android.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[sirnewton_01, 2014-07-15 03:52:03]

Hi All,

The test harness code came originally from go tools and appeared to be working
ok on my local machine. Is there a better way to validate changes and avoid
breaking builds?

Thanks,
Chris

> On Jul 14, 2014, at 9:55 PM, Dave Cheney <dave@cheney.net> wrote:
> 
> Yup, and it broke all the nacl builders as well, I guess we fake
> /etc/hosts for the hosts that we expect, and we didn't expect this
> one.
> 
>> On Tue, Jul 15, 2014 at 11:40 AM, minux <minux@golang.org> wrote:
>> 
>>> On Mon, Jul 14, 2014 at 9:36 PM, <gobot@golang.org> wrote:
>>> 
>>> This CL appears to have broken the android-arm-crawshaw builder.
>>> See http://build.golang.org/log/cc2dbc7029a8bb50fbae517633f05bcdc6b430dd
>> 
>> A real failure for android port.
>> 
>> --- FAIL: TestRepoRootForImportPath (0.65s)
>> vcs_test.go:109: RepoRootForImport("code.google.com/p/go"): Get
>> https://code.google.com/p/go/source/checkout?repo=: x509: failed to load
>> system roots and no roots provided
>> FAIL
>> 
>> we need some special handling for crypto/x509 on android.
>> 
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

[adg, 2014-07-15 04:30:22]

On 15 July 2014 13:51, Chris McGre <newton688@gmail.com> wrote:

> The test harness code came originally from go tools and appeared to be
> working ok on my local machine. Is there a better way to validate changes
> and avoid breaking builds?


It's not your fault. It's because nacl/android are strange environments
without a normal network.

Andrew

[dave_cheney.net, 2014-07-15 04:40:20]

https://codereview.appspot.com/113140043 should fix the problem, I can
backport this to the go.tools subrepo later.

On Tue, Jul 15, 2014 at 2:29 PM, Andrew Gerrand <adg@golang.org> wrote:
>
> On 15 July 2014 13:51, Chris McGre <newton688@gmail.com> wrote:
>>
>> The test harness code came originally from go tools and appeared to be
>> working ok on my local machine. Is there a better way to validate changes
>> and avoid breaking builds?
>
>
> It's not your fault. It's because nacl/android are strange environments
> without a normal network.
>
> Andrew
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.