Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/build/coordinator: make oauth2 build green, support GitHub / non-Go dependencies #14594

Closed
adg opened this issue Mar 2, 2016 · 17 comments
Closed

x/build/coordinator: make oauth2 build green, support GitHub / non-Go dependencies #14594

adg opened this issue Mar 2, 2016 · 17 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge
Milestone
Unreleased

Comments

@adg
Copy link
Contributor

adg commented Mar 2, 2016

The oauth2 repo is currently broken on the build dashboard because the coordinator doesn't know how to fetch dependencies external to the Go sub-repos.

../../gopath/src/golang.org/x/oauth2/google/default.go:20:2: cannot find package "google.golang.org/cloud/compute/metadata" in any of:
    /tmp/workdir/go/src/google.golang.org/cloud/compute/metadata (from $GOROOT)
    /tmp/workdir/gopath/src/google.golang.org/cloud/compute/metadata (from $GOPATH)

Options:

  1. Teach the coordinator how to fetch packages from repos other than the Go sub-repos.
    • I'm not in favor of this approach due to the complete lack of versioning (that already exists, but could be perceived as "worse" when we're talking about repos outside our control).
  2. Use a build tag to exclude packages from continuous build. Something like "gobuildfarm". Then packages that shouldn't be tested can specify // +build !gobuildfarm in their source files.
    • I feel somewhat ambivalent about this approach. It sets an odd precedent. It can be used to make the build green when it's actually not. That seems bad.
  3. Skip packages that have dependencies outside the Go project.
    • This seems promising. We already know which packages these are. The question is, how do we keep an eye on the packages we're skipping? We don't want to silently lose test coverage by adding an external dependency.
  4. Remove oauth2 from the build dashboard as it'll likely never be able to build using only the sub-repos.
    • This has some appeal, because there was no solid reason to make oauth2 a golang.org/x/ repo in the first place. Maybe it doesn't belong on the build dashboard.

My instinct points to options 3 or 2.

cc @bradfitz @rakyll
@adg adg self-assigned this Mar 2, 2016
@rakyll
Copy link
Contributor

rakyll commented Mar 3, 2016

Before answering your question, I wonder whether we should vendor the external packages in the oauth2 package. I think we should have never contributed provider-specific bits with external dependencies to the oauth2 package. They could have lived under a google package elsewhere. But since we have these bits and cannot break the existing users, it is too late to move the google package somewhere else. Vendoring will solve the broken build, right?

@adg
Copy link
Contributor Author

adg commented Mar 3, 2016

Yes, option 5 is vendoring the dependencies!

The only dependencies this repo has outside the standard library are:

golang.org/x/net/context
google.golang.org/cloud/compute/metadata
google.golang.org/cloud/internal

We should (and need) not vendor x/net/context, but vendoring the tiny cloud/compute/metadata package (and the dependent cloud/internal package) is probably worthwhile.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 3, 2016

That would break some people's rule that vendoring should only be done by binaries, and not libraries.

I need to solve this for grpc-go too, since I want to run their tests on our infrastructure. I'm thinking a manifest file of deps for the builder, similar to the .travis.yml file (but not yaml).

@kostya-sh
Copy link
Contributor

Could google.golang.org/cloud/compute/metadata and google.golang.org/cloud/internal be copied to golang.org/x/oauth2/internal with paths rewritten instead of vendoring them?

Alternatively it is possible to remove the dependency completely by copying a few functions (metadata.OnGCE, metadata.Get) that are needed from these external packages to oauth2 repo.

@bradfitz
Copy link
Contributor

bradfitz commented Mar 3, 2016

@kostya-sh, that's just as bad as vendoring them, or a bit worse. I'd rather fix our test infrastructure rather than mirror that code all over the place.

@rakyll
Copy link
Contributor

rakyll commented Mar 3, 2016

Fixing the infra SGTM.

@kostya-sh
Copy link
Contributor

@bradfitz, I suggested copying because:

  • Building and vendoring packages with less or no dependencies is simpler
  • The amount of code to copy is quite small (l think less than 100 lines)
  • I assumed that GCE metadata HTTP API is stable

@bradfitz bradfitz modified the milestone: Unreleased Apr 9, 2016
@dsymonds
Copy link
Contributor

I think option 1 is the best course of action, and we can probably get by for quite a while with a simple list of additional packages to fetch for a given subrepo. That will keep any addition "external" dependencies from quietly slipping in, and it's going to be rare to add/remove these "external" dependencies.

For oauth2, it just needs to run go get google.golang.org/cloud before building.

@adg
Copy link
Contributor Author

adg commented Apr 19, 2016

For oauth2, it just needs to run go get google.golang.org/cloud before building.

Rather than running go get we'd probably want to fetch the dependency manually (there's already some code to do this). That way the build would break if the cloud packages add transitive dependencies.

I know @bradfitz has thoughts on how to address this issue more generally.

@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Mar 21, 2017
@bradfitz bradfitz mentioned this issue Jul 21, 2017
Closed
@adg adg removed their assignment Feb 14, 2018
@bradfitz bradfitz changed the title x/build/coordinator: make oauth2 build green x/build/coordinator: make oauth2 build green, support GitHub / non-Go dependencies Aug 24, 2018
@bradfitz bradfitz mentioned this issue Jan 9, 2019
Closed
@bradfitz bradfitz self-assigned this Jan 10, 2019
@bradfitz
Copy link
Contributor

Now that we have modules (and that https://github.com/gomods/athens exists), this got almost trivial.

@bcmills is adding a go.mod file to oauth2 in https://go-review.googlesource.com/c/oauth2/+/157137 and then I'll run an Athens in Kubernetes:

And then just set GOPROXY when we run go test commands. Super trivial.

@gopherbot
Copy link

Change https://golang.org/cl/157438 mentions this issue: cmd/coordinator: support testing subrepos that use modules

@gopherbot
Copy link

Change https://golang.org/cl/157439 mentions this issue: cmd/buildlet: ignore symlinks for now

gopherbot pushed a commit to golang/build that referenced this issue Jan 11, 2019
@bradfitz
cmd/buildlet: ignore symlinks for now
c61dc90 
As part of getting the x/build tests passing (trybots + post-submit),
ignore the symlinks that are in the x/build repo for now.

We can add support when we actually need them for something. But I
imagine any test that needs them can & does just create them as
needed.

Updates golang/go#14594

Change-Id: I6a0584d35c8d0fc91b3cdc8114b473df7f0268c3
Reviewed-on: https://go-review.googlesource.com/c/157439
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Jan 14, 2019
@bradfitz
cmd/coordinator: support testing subrepos that use modules
d960d82 
Only the "oauth2" and "build" repos for now, as a test. We'll lock
down policy more later and decide when to do this automatically.

Also, this currently only runs buildlets which run in our GCP project,
because we're not yet proxying the a localhost:3000 port from the
reverse buildlets to an authenticated TLS connection back to our
module proxy service on GKE.

Updates golang/go#14594
Fixes golang/go#29637

Change-Id: I6f05da2186b38dc8056081252563a82c50f0ce05
Reviewed-on: https://go-review.googlesource.com/c/157438
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@bcmills bcmills mentioned this issue Jan 17, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/165779 mentions this issue: cmd/coordinator: add authenticated proxy to internal module proxy

gopherbot pushed a commit to golang/build that referenced this issue Mar 7, 2019
@bradfitz
cmd/coordinator: add authenticated proxy to internal module proxy
2f7a8c1 
So builders outside of GCE (Macs, etc) can use a module proxy. It will
be exposed to them via an unauthenticated localhost:3000 server that
adds the necessary authentication to this server.

This is a follow-up of work started in CL 157438.

Updates golang/go#14594

Change-Id: Id824b0ad3a0274048023cc72f8adad23d5f0ea29
Reviewed-on: https://go-review.googlesource.com/c/build/+/165779
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/165780 mentions this issue: cmd/coordinator: fix authentication to not require a user token

gopherbot pushed a commit to golang/build that referenced this issue Mar 7, 2019
@bradfitz
cmd/coordinator: fix authentication to not require a user token
f4aa239 
Buildlets have regular builder tokens, not "user-" prefixed ones. So
don't use the auth helper function. Just inline what we need in the
proxy handler.

Fix from testing CL 165779.

Updates golang/go#14594

Change-Id: Ie2d8d7a21f5660d24e929c932571b8df61895374
Reviewed-on: https://go-review.googlesource.com/c/build/+/165780
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/165782 mentions this issue: cmd/buildlet: add localhost proxy to farmer.golang.org's module proxy

gopherbot pushed a commit to golang/build that referenced this issue Mar 7, 2019
@bradfitz
cmd/buildlet: add localhost proxy to farmer.golang.org's module proxy
c78805d 
Now that farmer.golang.org is an authenticated GOPROXY server (CL 165779),
this adds the unauthenticated localhost server on the buildlet that
adds the auth headers to farmer.golang.org.

The buildlet only does this if the build includes env
GO_BUILDER_SET_GOPROXY=coordinator, in which case it listens on an
emphemeral localhost port per exec and sets GOPROXY to an HTTP server
running on that port. This way we don't need to deal with any port
management or conflicts.

Updates golang/go#14594

Change-Id: Iae6d2deda9d5e88ab659d94aaccc43e01fcf4a7c
Reviewed-on: https://go-review.googlesource.com/c/build/+/165782
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/166218 mentions this issue: dashboard: unify post-submit & trybot policy in builders.go, not coordinator

gopherbot pushed a commit to golang/build that referenced this issue Mar 8, 2019
@bradfitz
dashboard: unify post-submit & trybot policy in builders.go, not coor…
756764c 
…dinator

This removes a bunch of TODOs about scattered policy and moves it all
into builders.go.

While here,

* add a bunch of tests
* unexport some things
* rename some things
* document some things
* adjust FreeBSD policy as function of branch (per Go 1.12 being
  last to support FreeBSD 10.x, and to unblock CL 165801)
* set GO_BUILDER_SET_GOPROXY=coordinator for reverse buildlets,
  which is necessary to remove the oauth2 & build special cases
  in the config
* change Elias Naur's mobile builder to how I think he wants it
  (he was fighting the old system)
* add $HOME on the Solaris smartos builder, which was missing &
  causing tests to fail lately
* make the (currently failing) longtest builder have GOPROXY set
* remove an allocation in version.ParseReleaseBranch

Fixes golang/go#9603
Updates golang/go#14594

Change-Id: I50a23ad7cdf478c95b14bee9b3931ba361baacfa
Reviewed-on: https://go-review.googlesource.com/c/build/+/166218
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@bradfitz
Copy link
Contributor

This has been done for a number of months.

@dmitshur dmitshur mentioned this issue Jun 13, 2019
Closed
@bcmills bcmills mentioned this issue Aug 21, 2019
Closed
@golang golang locked and limited conversation to collaborators May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @dsymonds @rakyll @kostya-sh @adg @gopherbot