serving/samples: update helloworld-go to go:1.13 and go modules

[grayside]

First step toward #1686

Proposed Changes

• Upgrades from go 1.12 to go 1.13
• Upgrades to use go modules
• Pins alpine image to 3.10
• Tweaks working directory to /app
• Rename binary in container from helloworld to server
• Use go mod download to facilitate a layer cache of go deps.
• Use -mod=readonly flag on builds for strictness of container builds
• Some minor style changes around syntax and comments

[grayside]

/assign @ahmetb

[grayside]

/test pull-knative-docs-unit-tests

[ahmetb]

Looks good. Some nits.

[ahmetb]

Any particular reason not to use golang:1?

We have to keep updating this every 6 months otherwise.

[grayside]

Given that sometimes the minor version has significant change to best practices (e.g., modules) I think the clarity is worthwhile for the reader and motivation to update.

[tbpg]

I wonder if this should do the go mod download trick, since many people will copy this? I know it's been decided on in the past, but consider adding it.

(I'm not attached to adding it - definitely makes things more complex, but it might be worth it.)

[grayside]

Added go mod download and modified go build to use -mod=readonly for strictness of the build.

[grayside]

/test pull-knative-docs-integration-tests

[grayside]

Looking at the error, I see:

            Step 3/10 : COPY go.* ./
            COPY failed: no source files were specified

However, there is plainly a go.mod file in this sample and it builds locally just fine. Is it possible the test rig itself is skipping a file or missing the addition of go.mod somehow?

@chaodaiG could you take a look?

[chaodaiG]

let me TAL

[chizhg]

Looking at the error, I see:

            Step 3/10 : COPY go.* ./
            COPY failed: no source files were specified

However, there is plainly a go.mod file in this sample and it builds locally just fine. Is it possible the test rig itself is skipping a file or missing the addition of go.mod somehow?

@chaodaiG could you take a look?

@grayside you'll need to add go.mod in

docs/test/sampleapp/config.yaml

Line 14 in 533e6b7

- "helloworld.go"

Also as I tried in #1787, you'll also need to remove -mod=readonly from the go build command.

[chaodaiG]

Thank you @Fredy-Z for looking into this!

[grayside]

Thanks @Fredy-Z,

I want to make sure I understand what's happening here.

1. Why can't I use -mod=readonly? That seems like it should be the best practice in a container, especially where the deps are downloaded via go mod download? In this case, the test framework appears to be constraining the implementation of the app in unexpected ways.
2. Why doesn't the test process copy all the files, perhaps excluding just .dockerignore? In this case, the test framework seems to be defining unexpected conventions.

[chizhg]

Add an empty line after this line. Otherwise go build will not work with -mod=readonly.

[chizhg]

Thanks @Fredy-Z,

I want to make sure I understand what's happening here.

1. Why can't I use -mod=readonly? That seems like it should be the best practice in a container, especially where the deps are downloaded via go mod download? In this case, the test framework appears to be constraining the implementation of the app in unexpected ways.
2. Why doesn't the test process copy all the files, perhaps excluding just .dockerignore? In this case, the test framework seems to be defining unexpected conventions.

@grayside sorry for the late reply.

For 1, I double checked your PR, this reason that you cannot have -mod=readonly is because the go.mod file is not ended with an empty line, see #1774 (comment).
For 2, I'm not sure what's the historic reason for only copying specific files, to me it's totally fine to just copy all files. @chaodaiG can you take a look?

[chaodaiG]

Thanks @Fredy-Z,
I want to make sure I understand what's happening here.

1. Why can't I use -mod=readonly? That seems like it should be the best practice in a container, especially where the deps are downloaded via go mod download? In this case, the test framework appears to be constraining the implementation of the app in unexpected ways.
2. Why doesn't the test process copy all the files, perhaps excluding just .dockerignore? In this case, the test framework seems to be defining unexpected conventions.

@grayside sorry for the late reply.

For 1, I double checked your PR, this reason that you cannot have -mod=readonly is because the go.mod file is not ended with an empty line, see #1774 (comment).
For 2, I'm not sure what's the historic reason for only copying specific files, to me it's totally fine to just copy all files. @chaodaiG can you take a look?

A piece of background, I have observed that all files needed for tests are already inlined in README.md files, so there is a unit test verifying the consistency between committed code and README, and the unit test goes through all files needs to be copied and verify if they are identical with README.md. See test here:

docs/test/sampleconsistency/sampleconsistency_test.go

Line 96 in 533e6b7

for _, f := range lc.Copies {

[grayside]

For -mod=readonly, the container builds fine for me locally as-is, not sure what I'd change there. Why is go build in the container different in CI here?

For file-copy: It makes sense to me that we test that files we include in the README are consistent with source code, but that should not determine what is copied. For example, there are lockfiles such as package-lock.json for Node.js which need to be included in the build.

[grayside]

Updated to add the go.mod to the test config, and updated the README to add an instruction on how to generate go.mod. Note that I am not copying the go.mod file into the README.

[chaodaiG]

For -mod=readonly, the container builds fine for me locally as-is, not sure what I'd change there. Why is go build in the container different in CI here?

For file-copy: It makes sense to me that we test that files we include in the README are consistent with source code, but that should not determine what is copied. For example, there are lockfiles such as package-lock.json for Node.js which need to be included in the build.

For file-copy, I'm fine with any change to make it smarter. One thing to keep in mind is, currently it's very tightly enforced to make sure all files needed for compilation are inspected in unit+e2e tests, if we keep separated list of what's copied and what's verified, there is a risk of these 2 lists diverged. So for any example I would recommend inline the content in README, unless it doesn't make sense at all. And in the latter case there is a way to get around it(not highly encouraged though) by running custom cp command, see example for Spring here:

docs/test/sampleapp/config.yaml

Line 4 in 533e6b7

preCommands:

[grayside]

It looks like the test is now passing, which tells me that at least some of what we were discussing was a misunderstanding that the tests require inlining all files.

[chaodaiG]

that would be a bug if that's the case. We can have this PR pass and I'll TAL

[chaodaiG]

/approve

[grayside]

@chaodaiG there are files that are required for the build but are not and should not be inlined in the READMEs, for example, package-lock.json for Node.js. If the build leaves these out, they may pass but the result will be non-deterministic.

[chaodaiG]

I agree that there are certain exceptions like in package.json case, which as I mentioned earlier can get exempted by adding a manual cp step in preCommand.

[samodell]

/approve
/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, grayside, samodell, tbpg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [samodell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment