Update go.mod to specify the module is go1.14 #866

dprotaso · 2020-05-29T21:04:43Z

Thus the go commands will default to -mod=vendor

See: https://golang.org/doc/go1.14#go-command

dprotaso · 2020-05-29T22:36:55Z

I dunno what's up with the build test - the scripts have deviated from other repos

-----------------------------------------
---- Checking for forbidden licenses ----
-----------------------------------------
---- Fri May 29 15:18:50 PDT 2020
-----------------------------------------
Step failed: default_build_test_runner
============================
==== BUILD TESTS FAILED ====
============================
==== Fri May 29 15:19:20 PDT 2020
============================
Step failed: run_build_tests

rhuss

Thanks for the PR, but I wonder whether the motivation is only to use vendoring as we already have vendoring enabeld for go modules since quite some time (e.g.

client/hack/build.sh

Line 133 in d8a9692

go build -mod=vendor -ldflags "$(build_flags $(basedir))" -o kn ./cmd/...

)

What concrete problem is this PR solving ?

rhuss · 2020-05-31T15:43:17Z

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.6.2
 	golang.org/x/crypto v0.0.0-20200302210943-78000ba7a073
+	golang.org/x/tools v0.0.0-20200329025819-fd4102a86c65


ok, I see. It's for goimports used during build. Does go has the notion of a "developer dependency" (like npm or Maven) ? I'm not big fan to mix in dependencies here which are not part of the application itself.

rhuss · 2020-05-31T15:44:06Z

hack/tools.go

@@ -0,0 +1,23 @@
+// +build tools


why is this file needed ?

I see, its that you can keep the depedency in go.mod on goimport so that it can be vendored and is not accidentally removed.

It still feels very hackish to me and is a clear smell, that dev dependencies should be managed separately (but it looks like its a go limitation).

I could see two solutions here:

Don't compile devtools like "goimports" on the fly but use a given binary (like from a container image)

Use an own kind of vendor by committing dev dependencies in a different vendor dire (vendor_dev) and fiddle with GOPATH before calling go get goimports.

I agree that using goimports as it is now is not the correct way, also because the go get call will update modules.txt which sometimes leads to a diff to the comitted modules.txt when running in the CI. Up to now fixed the modules.txt manually in such a case, but vendoring seems to be the better solution here.

If this does not work out or is too involved, than we can of course vendor goimports.

But we should keep -mod=vendor for all go commands everywhere to be explicit and not relying on defaults which can change (which seems to happen quite a bit in go-land as just seen when going from go 1.13 to 1.14)

maximilien

Elaborate a bit on the change as per @rhuss and also update CHANGELOG.md

dprotaso · 2020-06-05T01:46:45Z

go1.14 with the presence of a vendor folder will always use -mod=vendor for various go commands.

Your verify-codegen.sh tries to install the package but it's missing from vendor. To vendor it we need to reference the package being installed - the convention used is to have a tools.go file.

dprotaso · 2020-06-05T01:47:03Z

Otherwise te error looks like

# ./hack/verify-codegen.sh
can't load package: package golang.org/x/tools/cmd/goimports: cannot find package "." in:
	/go/client/vendor/golang.org/x/tools/cmd/goimports

dprotaso · 2020-06-05T01:56:08Z

weird - update licenses seems broken

dprotaso · 2020-06-05T02:03:36Z

Other option if you don't want to pin the tool to a specific sha is to use test-infra's run_go_tool

dprotaso · 2020-06-05T02:07:34Z

so I think go code is getting pulled into third_party if it's unable to determine the license for the file.

started a thread here: https://knative.slack.com/archives/CA1DTGZ2N/p1591322638315800

Probably not worth conflating this PR with that concern.

dprotaso · 2020-06-05T02:11:09Z

This should be good now - if you want me to drop vendoring goimports let me know and I can switch to the go_run_tool option

test/presubmit-tests.sh

hack/build.sh

rhuss · 2020-06-05T06:31:14Z

This should be good now - if you want me to drop vendoring goimports let me know and I can switch to the go_run_tool option

@dprotaso If this avoids messing with the client's go.mod this would be then the preferred solution (if this also works locally outside the CI)

This reverts commit 9616d5e.

dprotaso · 2020-06-08T14:51:53Z

rebased since there was a merge conflict

rhuss · 2020-06-08T15:16:16Z

Thanks ! Let me check later ...

rhuss

thanks, your PR looks small and elegant. But until test-infra is not running on macOs' bash v3 we can reuse the code in the main development script (see comments)

rhuss · 2020-06-08T18:00:05Z

hack/build.sh

@@ -16,6 +16,8 @@

 set -o pipefail

+source $(dirname $0)/../scripts/test-infra/library.sh


Unfortunately, that won't work for macOS. See knative/test-infra#2143

build.sh is supposed to run on any Unix like OS, also macOS, without any other dependencies. So having Bash 4 dependency here is a no-go, so we can include any test-infra scripts in build.sh.

rhuss · 2020-06-08T18:02:40Z

hack/build.sh

-     find $(echo $source_dirs) -name "*.go" -print0 | xargs -0 gofmt -s -w
-  fi
+  run_go_tool golang.org/x/tools/cmd/goimports goimports -w $(echo $source_dirs)
+  find $(echo $source_dirs) -name "*.go" -print0 | xargs -0 gofmt -s -w
  set -e


Sorry for my former comment about using run_go_tool as I haven't known that the test-infra scripts are not running on the stock bash of macOS.

However, I wonder whether we could inline the run_go_tool code here, making it running on macOs ?

And even is run_go_tool happens to run on macOs (by accident), I still wouldn't include library.sh as long as there is a strong commitment to stick to bash version 3 syntax only.

dprotaso · 2020-06-08T19:33:59Z

inlined

maximilien

/lgtm

knative-prow-robot · 2020-06-08T20:40:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, maximilien

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~~ [maximilien]

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

rhuss · 2020-06-08T23:28:26Z

hack/build.sh

@@ -16,6 +16,9 @@

 set -o pipefail

+[[ ! -v REPO_ROOT_DIR ]] && REPO_ROOT_DIR="$(git rev-parse --show-toplevel)"


@dprotaso It's that -v which does not work in bash v3

~~No worries, I'll fix that tomorrow~~ day after tomorrow, sorry.

rhuss · 2020-06-09T11:40:39Z

/lgtm

@maximilien why ? It hasn't been tested on macOS (as requested) and breaks now the build for every macOs user.

rhuss · 2020-06-09T12:01:58Z

inlined

@dprotaso I meant "inline the logic of that function", not to inline it literally, as this is not better than sourcing library.sh (as seen in #882 )

* Update go.mod to specify the module is go1.14 Thus the go commands will default to -mod=vendor See: https://golang.org/doc/go1.14#go-command * install goimports as a tool * drop mod=vendor usage as that's the default with go1.14 * remove comment about using -mod=vendor * Revert "drop mod=vendor usage as that's the default with go1.14" This reverts commit 567004d. * Revert "remove comment about using -mod=vendor" This reverts commit 2a71393. * Revert "install goimports as a tool" This reverts commit 9616d5e. * use go_run_tool to run goimports * inline go_run_tool * include required env var REPO_ROOT_DIR

dprotaso · 2020-06-11T17:47:28Z

inlined

@dprotaso I meant "inline the logic of that function", not to inline it literally, as this is not better than sourcing library.sh (as seen in #882 )

I see my mistake - sorry for the trouble

rhuss · 2020-06-12T06:53:38Z

I see my mistake - sorry for the trouble

no worries, the fix was trivial :). Thanks for working on aligning the Knative repos !

* Update go.mod to specify the module is go1.14 Thus the go commands will default to -mod=vendor See: https://golang.org/doc/go1.14#go-command * install goimports as a tool * drop mod=vendor usage as that's the default with go1.14 * remove comment about using -mod=vendor * Revert "drop mod=vendor usage as that's the default with go1.14" This reverts commit 567004d. * Revert "remove comment about using -mod=vendor" This reverts commit 2a71393. * Revert "install goimports as a tool" This reverts commit 9616d5e. * use go_run_tool to run goimports * inline go_run_tool * include required env var REPO_ROOT_DIR

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880 knative#883

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for #866 #880 #883

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 29, 2020

knative-prow-robot requested review from evankanderson and maximilien May 29, 2020 21:04

knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 29, 2020

dprotaso force-pushed the patch-1 branch from 9e66c19 to 2c436e1 Compare May 29, 2020 22:16

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 29, 2020

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 29, 2020

rhuss reviewed May 31, 2020

View reviewed changes

maximilien reviewed Jun 2, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2020

dprotaso force-pushed the patch-1 branch from 14400e7 to 94a3495 Compare June 5, 2020 01:50

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2020

dprotaso force-pushed the patch-1 branch from 94a3495 to e038fd3 Compare June 5, 2020 01:53

mattmoor reviewed Jun 5, 2020

View reviewed changes

dprotaso force-pushed the patch-1 branch from 453a90a to 2a71393 Compare June 5, 2020 01:55

rhuss reviewed Jun 5, 2020

View reviewed changes

test/presubmit-tests.sh Outdated Show resolved Hide resolved

rhuss reviewed Jun 5, 2020

View reviewed changes

hack/build.sh Outdated Show resolved Hide resolved

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 5, 2020

dprotaso added 2 commits June 8, 2020 10:51

Revert "install goimports as a tool"

926681d

This reverts commit 9616d5e.

use go_run_tool to run goimports

43644de

dprotaso force-pushed the patch-1 branch from 9fc2583 to 43644de Compare June 8, 2020 14:51

rhuss reviewed Jun 8, 2020

View reviewed changes

inline go_run_tool

47eff26

include required env var REPO_ROOT_DIR

885bceb

maximilien approved these changes Jun 8, 2020

View reviewed changes

knative-prow-robot assigned maximilien Jun 8, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2020

knative-prow-robot merged commit a7ea77d into knative:master Jun 8, 2020

dprotaso deleted the patch-1 branch June 8, 2020 20:42

rhuss reviewed Jun 8, 2020

View reviewed changes

rhuss mentioned this pull request Jun 9, 2020

build.sh is broken for macOs users #882

Closed

navidshaikh mentioned this pull request Jun 16, 2020

v0.15.2 release backports #886

Merged

navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

9727fdc

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

navidshaikh added a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

a02b444

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

navidshaikh added a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

88f15b4

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880 knative#883

knative-prow-robot pushed a commit that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release (#886)

efc9df1

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for #866 #880 #883

navidshaikh added the backported-to/0.15 label Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update go.mod to specify the module is go1.14 #866

Update go.mod to specify the module is go1.14 #866

dprotaso commented May 29, 2020

dprotaso commented May 29, 2020

rhuss left a comment

rhuss May 31, 2020

rhuss Jun 5, 2020

rhuss May 31, 2020

rhuss Jun 5, 2020

rhuss Jun 5, 2020

maximilien left a comment

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020 •

edited

Loading

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

rhuss commented Jun 5, 2020

dprotaso commented Jun 8, 2020

rhuss commented Jun 8, 2020

rhuss left a comment

rhuss Jun 8, 2020

rhuss Jun 8, 2020

dprotaso commented Jun 8, 2020

maximilien left a comment

knative-prow-robot commented Jun 8, 2020

rhuss Jun 8, 2020 •

edited

Loading

rhuss commented Jun 9, 2020

rhuss commented Jun 9, 2020

dprotaso commented Jun 11, 2020

rhuss commented Jun 12, 2020

		@@ -16,6 +16,8 @@

		set -o pipefail

		source $(dirname $0)/../scripts/test-infra/library.sh

		@@ -16,6 +16,9 @@

		set -o pipefail

		[[ ! -v REPO_ROOT_DIR ]] && REPO_ROOT_DIR="$(git rev-parse --show-toplevel)"

Update go.mod to specify the module is go1.14 #866

Update go.mod to specify the module is go1.14 #866

Conversation

dprotaso commented May 29, 2020

dprotaso commented May 29, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss May 31, 2020

Choose a reason for hiding this comment

rhuss Jun 5, 2020

Choose a reason for hiding this comment

rhuss May 31, 2020

Choose a reason for hiding this comment

rhuss Jun 5, 2020

Choose a reason for hiding this comment

rhuss Jun 5, 2020

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020 • edited Loading

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

dprotaso commented Jun 5, 2020

rhuss commented Jun 5, 2020

dprotaso commented Jun 8, 2020

rhuss commented Jun 8, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss Jun 8, 2020

Choose a reason for hiding this comment

rhuss Jun 8, 2020

Choose a reason for hiding this comment

dprotaso commented Jun 8, 2020

maximilien left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 8, 2020

rhuss Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

rhuss commented Jun 9, 2020

rhuss commented Jun 9, 2020

dprotaso commented Jun 11, 2020

rhuss commented Jun 12, 2020

dprotaso commented Jun 5, 2020 •

edited

Loading

rhuss Jun 8, 2020 •

edited

Loading