Go: x/tools/godoc: Dockerize and convert to run on GAE Flex

[andybons]

godoc relies on https://golang.org/doc/go1.10#goroot but GAE Classic is still running Go 1.9.

Convert to use GAE Flex so that we can rid ourselves of these limitations (and deploy godoc from the release-branch.go1.11)

/cc @bradfitz @dmitshur @katiehockman

[bradfitz]

Milestone: Go 1.6 :)

[bradfitz]

Also, because we're using a Go 1.10 x/tools on golang.org now, certain links don't work from the release notes & blog post, like:

https://golang.org/cmd/go/#hdr-Modules__module_versions__and_more

Because the godoc heading rules. This should be a heading:

Modules, module versions, and more

But it renders as a plain paragraph at the moment.

[dmitshur]

Similarly, this statement from https://golang.org/doc/go1.11#godoc is not yet true on https://golang.org/pkg/:

The godoc web server now shows which version of Go introduced new API features.

As a workaround, in the meantime, you can refer to https://tip.golang.org/pkg/. E.g.:

https://tip.golang.org/pkg/os/#UserCacheDir

[steren]

FYI, Go 1.11 on the App Engine standard environment is coming.

[bradfitz]

@steren, that doesn't help us. App Engine's Go runtime by definition will always be behind what we need, because we have to release Go before App Engine can release a Go runtime for that version. So we can't use App Engine's provided runtimes. We can only do a custom runtime (BYODockerfile).

So we're moving off App Engine classic.

[steren]

I see, yes in that case, a Dockerfile approach seems to be appropriate.

[broady]

Checklist of things to do.

• [x] Create Dockerfile
• [x] Update deployment process/script to use env: flex
• [x] Consider introducing another build flag to include "golang.org" stuff (short, dl). Currently the "appengine" build tag is used for this (see appinit.go). An environment variable might be even better.

Done already: networking/routing. We did this work already about a year ago.

Cannot use "google.golang.org/appengine" packages in Flex, so these must be migrated. Significant packages/code that needs to be updated:

• [x] cmd/godoc/appinit.go (main entrypoint for golang.org)
• [x] godoc/dl (downloads page uses datastore/memcache)
• [x] godoc/short (short URL facility, uses datastore/memcache)

I guess the main questions are what we use to replace the memcache bits in dl and short. It's probably right to use golang.org/x/playground's cache.

Relevant deps of cmd/godoc, which will all need to be checked for any appengine stuff:

golang.org/x/tools/blog
golang.org/x/tools/blog/atom
golang.org/x/tools/godoc
golang.org/x/tools/godoc/analysis
golang.org/x/tools/godoc/dl
golang.org/x/tools/godoc/proxy
golang.org/x/tools/godoc/redirect
golang.org/x/tools/godoc/short
golang.org/x/tools/godoc/static
golang.org/x/tools/godoc/util
golang.org/x/tools/godoc/vfs
golang.org/x/tools/godoc/vfs/httpfs
golang.org/x/tools/godoc/vfs/mapfs
golang.org/x/tools/godoc/vfs/zipfs
golang.org/x/tools/present

[gopherbot]

Change https://golang.org/cl/133355 mentions this issue: godoc: migrate to App Engine flexible

[broady]

One more thing we need to do: move the shortlink admin service to another GAE app or just a local command.

It uses appengine/user to check the viewer has admin access. There's no simple migration for that (need to set up OAuth/Firebase Auth).

[bradfitz]

How about we migrate the shortlinks to a plain text file stored in git?

The admin service is broken even for classic App Engine on custom domains anyway and when I want to add a new link, I spend more time remembering/finding the ugly workaround URL than I would if I just TBR'd a CL to a data file.

We can cache the data file in memory and on cache miss (for new links) fault to Gerrit for fetching the file at HEAD, rate limited to once every 5 minutes or so.

[broady]

I like that, it also gives non-admins a list of available shortlinks.

Where should the list live? x/tools/godoc/short/links.tsv?

[bradfitz]

That works. But slight preference for links.txt(or links.dat) instead and not requiring tabs. I'd just use any whitespace. Ignore comment lines starting with '#' otherwise just use strings.Fields and get first element is short key and second is full URL.

[bradfitz]

It's now been over a month for this "Soon" bug.

What remains?

[andybons]

Ping @broady, who last I heard had a working version locally and was working on the deploy scripts. He's in Sydney at the moment so there may be some delay, though.

[broady]

Update... what remains:

• [x] porting over the patches in existing deployment scripts:
  
  
  
  • [x] google analytics
  
  
  • [x] hash.map/hash.go for mercurial -> git redirects
  
  
• [x] package listing is missing, but package docs work just fine (this is because the package listing looks at GOROOT/pkg, which is empty because I do a clean checkout of the git repo)

Fast-follow:

• [ ] ability to add shortlinks (old app had a login, GAE Flex doesn't have authed handlers)
• [x] update hostname filtering to allow newly deployed versions - so we can smoke test before migrating traffic to the new version (hostnames look like 20180907t155022-dot-golang-org.appspot.com) - currently controlled with GODOC_ENFORCE_HOSTS env var.
• [x] port over other stuff from internal deployment scripts:
  
  
  
  • [x] regression tests
  
  
  • [x] version number prettification
  
  
• [ ] remove zipfs (#27959)
• [ ] remove zipping from build process (generate-index.bash)
• [x] build a given Go version from scratch in the builder phase, rather than using FROM golang

[broady]

Update: well, I sank several hours into figuring out why the package listing was empty.

It looks like there have been numerous changes to x/tools/godoc/vfs, my best guess is CL 101295.

Precisely, this change to zipfs:
https://github.com/golang/tools/commit/16d1af8d88c08b6a22e07a0900aad5279a7fb4fd#diff-6b09edd71f61887ea342b82eb207149b

/cc @agnivade

[agnivade]

Yes, I made that change. But it was also fixed later with https://github.com/golang/go/issues/27162. Are you sure you have the latest 1.11 ?

So this comment -

this is because the package listing looks at GOROOT/pkg, which is empty because I do a clean checkout of the git repo

is not true anymore. Package listing will look at the resolved goroot now - which should be the bin/ folder inside a go distribution where godoc, and go are kept together. Plus, if there is any confusion, you can pass the -goroot flag yourself.

Let me know if I can be of any further help.

[broady]

Yes, I made that change. But it was also fixed later with #27162. Are you sure you have the latest 1.11 ?

It's broken for zipfs. It works fine on real filesystem. See the exact lines linked two posts above.

"abspath" is a path to a file, not a directory, so it'll almost never match. So, RootType is almost always empty, which results in an empty entry in the package list. (strangely, a <tr> is still emitted.)

Anyway, this is a discussion for a new issue, not this one.

[agnivade]

Ah, my apologies for this ! Will send a fix.

[gopherbot]

Change https://golang.org/cl/138435 mentions this issue: godoc/vfs/zipfs: join paths to get correct RootType

[gopherbot]

Change https://golang.org/cl/138978 mentions this issue: cmd/godoc: improve deployment scripts, add buildinfo

[gopherbot]

Change https://golang.org/cl/139237 mentions this issue: cmd/godoc: re-enable host checking, allow test versions

[gopherbot]

Change https://golang.org/cl/139238 mentions this issue: cmd/godoc: move regression tests to a go test

[agnivade]

Now that site is live, is there anything else pending on this ?

[broady]

@agnivade yes, see the unchecked items in the thread.

I'm also going to:

• [x] build Docker image using Cloud Build, so the only dependency (other than Make) for the person deploying is gcloud. At present, the person deploying must have Docker installed.

[gopherbot]

Change https://golang.org/cl/139239 mentions this issue: cmd/godoc: addmake publishto migrate traffic

[agnivade]

Ah thanks. Though I think removing zipfs is orthogonal to this issue, and should not block finishing this task.

[gopherbot]

Change https://golang.org/cl/139240 mentions this issue: cmd/godoc: add cloud build config

[agnivade]

@broady - I still don't see the version tags that are supposed to appear.

See - https://tip.golang.org/pkg/net/http/#SameSite vs https://golang.org/pkg/net/http/#SameSite. There is a "1.11" in the previous. Am I missing something ?

[broady]

Yes, I was wondering about that, too. I haven't debugged it, yet.

edit: I think I found it...

    // Initialize the version info before readTemplates, which saves
    // the map value in a method value.
    corpus.InitVersionInfo()

This needs to be added to appinit.go.

I'll add it.

edit: see golang.org/cl/139557

[broady]

One more...

• [ ] remove build tag from main.go (i.e., merge appinit.go and main.go)

[dmitshur]

It seems this issue is mostly resolved, with a minor cleanup followup task left. We should either remove the Soon label, since I don't think it applies now, or close this and open a new issue for the remaining cleanup task.

(/cc @andybons since you added the Soon label.)

[andybons]

Removed soon label

[gopherbot]

Change https://golang.org/cl/150599 mentions this issue: [release-branch.go1.11] godoc: migrate to App Engine flexible

[gopherbot]

Change https://golang.org/cl/150678 mentions this issue: [release-branch.go1.11] cmd/godoc: improve deployment scripts, add buildinfo

[gopherbot]

Change https://golang.org/cl/150679 mentions this issue: [release-branch.go1.11] cmd/godoc: re-enable host checking, allow test versions

[gopherbot]

Change https://golang.org/cl/150682 mentions this issue: [release-branch.go1.11] cmd/godoc: add cloud build config

[gopherbot]

Change https://golang.org/cl/150681 mentions this issue: [release-branch.go1.11] cmd/godoc: addmake publishto migrate traffic

[gopherbot]

Change https://golang.org/cl/150680 mentions this issue: [release-branch.go1.11] cmd/godoc: move regression tests to a go test

[dmitshur]

The task described in the original issue has long been complete. This issue has remained opened because there was one remaining clean up task (see https://github.com/golang/go/issues/27205#issuecomment-435172401):

• [ ] remove build tag from main.go (i.e., merge appinit.go and main.go)

The work happening in #29206 obsoletes the need for this to be done. /cc @cnoellekb After the golang.org website is deployed from x/website, x/tools/cmd/godoc will be reduced in scope to just serve documentation locally, and its appinit.go will be simply deleted.

That work is better tracked by other open issues, so I'll close this as done.

[agnivade]

There were a few others also which weren't done.

• [ ] remove zipping from build process (generate-index.bash)
• [ ] ability to add shortlinks (old app had a login, GAE Flex doesn't have authed handlers)

Not sure if they would be obsoleted too.

[dmitshur]

Thanks for pointing that out, I didn't see those two.

• [ ] remove zipping from build process (generate-index.bash)

I think this one will either be obsolete or will naturally come up as part of the post-move cleanup process. I don't see a need to track it via a dedicated issue, but if someone wants to, feel free to open one. Edit: This opportunity is now captured in an open issue at https://github.com/golang/go/issues/29206#issuecomment-536099768.

• [ ] ability to add shortlinks (old app had a login, GAE Flex doesn't have authed handlers)

I was going to open a new issue for that. I saw that https://golang.org/s pointed to this issue, but didn't find that task which related for it. Opened #29988 to track this.

[gopherbot]

Change https://golang.org/cl/160377 mentions this issue: godoc/short: point to new tracking issue for shortlink creation

[gopherbot]

Change https://golang.org/cl/160477 mentions this issue: [release-branch.go1.11] godoc/short: point to new tracking issue for shortlink creation