Clean the GO mod files by removing the unnecessary indirect imports #1454

camilamacedo86 · 2020-04-01T00:53:41Z

Description

Clean the GO mod files by removing the unnecessary indirect imports
fix make generate target to do cause the scenario again.

camilamacedo86 · 2020-04-01T18:13:26Z

/test pull-kubebuilder-e2e-k8s-1-16-2

Adirio · 2020-04-02T07:02:31Z

My understanding of go mod is pretty basic, I just know what it is needed to use them, but not the internals, so I'm not sure if those indirect imports are needed or not.

P.S.: Checking that tests pass in this case doesn't seem enough because probably none of those of those libraries made a backwards incompatible change so removing the indirect requirement may just get the last version and still work. But in case they make a backwards incompatible change it may break the build.

camilamacedo86 · 2020-04-02T12:34:18Z

/assign @mengqiy

mengqiy · 2020-04-02T18:49:20Z

I'd trust the go modules to do the job of tracking necessary entries in the go.mod file.
@camilamacedo86 What you can try is that first remove indirect entries and then run go mod tidy. If go modules add some entries back, then we probably should keep them.

camilamacedo86 · 2020-04-02T19:28:11Z

Hi @mengqiy,

Unfortunately, go mod tidy has been not removing the unnecessary imports from the go.mod. Note that shows that it has some issues opened for it. May solved for 1.4+?. (E.g golang/go#33008 )

See for example that "github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e" is not required in the main module.

$ go mod why github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e
# github.com/niemeyer/pretty
sigs.k8s.io/kubebuilder/internal/config
sigs.k8s.io/yaml
gopkg.in/yaml.v2
gopkg.in/yaml.v2.test
gopkg.in/check.v1
github.com/niemeyer/pretty

# v0.0.0-20200227124842-a10e7caefd8e
(main module does not need package v0.0.0-20200227124842-a10e7caefd8e)
camilamacedo@Camilas-MacBook-Pro ~/go/src/sigs.k8s.io/kubebuilder (master) $

Also, note that these indirect imports are just added when we run make generated. And then, see that all indirect imports are in the same situation.

# v0.0.0-20200301022130-244492dfa37a
(main module does not need package v0.0.0-20200301022130-244492dfa37a)

# v0.0.0-20200302083256-062a44052db1
(main module does not need package v0.0.0-20200302083256-062a44052db1)

# v0.0.0-20191204190536-9bdfabe68543
(main module does not need package v0.0.0-20191204190536-9bdfabe68543)

# v1.0.0-20200227125254-8fa46927fb4f
(main module does not need package v1.0.0-20200227125254-8fa46927fb4f)

$ go mod why gopkg.in/yaml.v2 v2.2.8 // indirect
# gopkg.in/yaml.v2
sigs.k8s.io/kubebuilder/internal/config
sigs.k8s.io/yaml
gopkg.in/yaml.v2

# v2.2.8
(main module does not need package v2.2.8)

# /
(main module does not need package /)

# indirect
(main module does not need package indirect)

Is it make sense @mengqiy? Are you ok with we clean it?

mengqiy · 2020-04-02T19:48:19Z

Also, note that these indirect imports are just added when we run make generated.

Though we do go mod tidy in

kubebuilder/Makefile

Line 50 in 29d1dc1

go mod tidy

, if go get in

kubebuilder/Makefile

Line 63 in 29d1dc1

- GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/[email protected]

still cause the problem, we should do go get in a temp directory. This can avoid it messing up the go.mod file.

camilamacedo86 · 2020-04-09T14:45:20Z

HI @mengqiy,

Now the go mod is clean the make generate target is fixed. I mean it will no longer install the indirect modules.

mengqiy · 2020-04-10T23:33:41Z

Makefile

-.PHONY: generate-setup
-generate-setup: ## Current workarround to generate the testdata with the correct controller-gen version
-	- rm -rf $(CONTROLLER_GEN_BIN_PATH)
-	- GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/[email protected]


You don't need to remove this section. I imagine that what you can do here is

create a temp directory (e.g. using https://golang.org/pkg/io/ioutil/#TempDir)

cd to that directory

run go get with go modules on

cd to the previous working directory

remove the temp dir.

This makefile target was added just to ensure that the correct version will be used.
Note that we just need to remove any pre-existent installation to ensure that. And then, the defined version in the makefile which will be scaffolded will be installed and used. So, it is not required at all and because of this, I keep just the rm -rf line and did not the tmp dir. See: https://github.com/kubernetes-sigs/kubebuilder/pull/1454/files#diff-b67911656ef5d18c4ae36cb6741b7965R57 Is it make sense?

mengqiy · 2020-04-13T16:30:53Z

Makefile

@@ -45,7 +52,9 @@ install: ## Build and install the binary with the current source code. Use it to

 .PHONY: generate
 generate: ## Update/generate all mock data. You should run this commands to update the mock data after your changes.
-	make generate-setup
+	# if an controller-gen be locally installed already it will be removed to ensure that the generate will
+	# get and use the correct version defined in the testdata makefiles instead of an previous installation.


This statement doesn't match what is done.
controller-gen is not installed after removing.

HI @mengqiy.

See:

When we start the controller-gen removed

And then in it got in the by the scaffold files

And then the gen installed when it finishes.

So, what is your suggestion for the comment/explanation?

Ha... You rely on the Makefile in the generated project to install the controller-gen for you.
I didn't realized it. It's not intuitive, but seems to be OK.

Please update the comment here to reflect how it work.
Then it should be OK to go.

Let me know if you are ok with the updates in the comment. Really tks for your review,

mengqiy

/lgtm
/approve

k8s-ci-robot · 2020-04-13T21:56:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mengqiy

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~~ [camilamacedo86,mengqiy]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 1, 2020

k8s-ci-robot requested review from Adirio and mengqiy April 1, 2020 00:53

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2020

camilamacedo86 mentioned this pull request Apr 1, 2020

Implement Wrapper interface for errors #1349

Merged

camilamacedo86 closed this Apr 1, 2020

camilamacedo86 reopened this Apr 1, 2020

camilamacedo86 closed this Apr 1, 2020

camilamacedo86 reopened this Apr 1, 2020

camilamacedo86 force-pushed the clean-imports branch from edbc724 to 8ac09ae Compare April 1, 2020 17:59

camilamacedo86 mentioned this pull request Apr 1, 2020

GitHub is not getting the result of the Travis #1457

Closed

camilamacedo86 closed this Apr 1, 2020

camilamacedo86 reopened this Apr 1, 2020

camilamacedo86 closed this Apr 2, 2020

camilamacedo86 reopened this Apr 2, 2020

camilamacedo86 force-pushed the clean-imports branch from 8ac09ae to f3f308d Compare April 2, 2020 15:00

camilamacedo86 changed the title ~~remove indirect imports~~ Remove the indirect imports and clean the GO mod file. Apr 2, 2020

camilamacedo86 force-pushed the clean-imports branch 2 times, most recently from d9555e8 to f64df06 Compare April 2, 2020 15:05

camilamacedo86 changed the title ~~Remove the indirect imports and clean the GO mod file.~~ Clean the GO mod files by removing the unnecessary indirect imports Apr 2, 2020

camilamacedo86 force-pushed the clean-imports branch from f64df06 to c7e081e Compare April 9, 2020 14:44

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2020

mengqiy reviewed Apr 10, 2020

View reviewed changes

mengqiy reviewed Apr 13, 2020

View reviewed changes

clean go mod and fix make generate target

8985369

camilamacedo86 force-pushed the clean-imports branch from c7e081e to 8985369 Compare April 13, 2020 20:22

mengqiy approved these changes Apr 13, 2020

View reviewed changes

k8s-ci-robot assigned mengqiy Apr 13, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2020

k8s-ci-robot merged commit c8d4a3d into kubernetes-sigs:master Apr 13, 2020

camilamacedo86 deleted the clean-imports branch April 13, 2020 23:45

camilamacedo86 mentioned this pull request Apr 20, 2020

Plugins Phase 1 #1469

Merged

1 task

camilamacedo86 mentioned this pull request Feb 18, 2021

New Release - proposing 3.0.0 stable #2029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean the GO mod files by removing the unnecessary indirect imports #1454

Clean the GO mod files by removing the unnecessary indirect imports #1454

camilamacedo86 commented Apr 1, 2020 •

edited

Loading

camilamacedo86 commented Apr 1, 2020

Adirio commented Apr 2, 2020

camilamacedo86 commented Apr 2, 2020 •

edited

Loading

mengqiy commented Apr 2, 2020

camilamacedo86 commented Apr 2, 2020 •

edited

Loading

mengqiy commented Apr 2, 2020

camilamacedo86 commented Apr 9, 2020

mengqiy Apr 10, 2020

camilamacedo86 Apr 11, 2020 •

edited

Loading

mengqiy Apr 13, 2020

camilamacedo86 Apr 13, 2020 •

edited

Loading

mengqiy Apr 13, 2020

mengqiy Apr 13, 2020

camilamacedo86 Apr 13, 2020 •

edited

Loading

mengqiy left a comment

k8s-ci-robot commented Apr 13, 2020

Clean the GO mod files by removing the unnecessary indirect imports #1454

Clean the GO mod files by removing the unnecessary indirect imports #1454

Conversation

camilamacedo86 commented Apr 1, 2020 • edited Loading

camilamacedo86 commented Apr 1, 2020

Adirio commented Apr 2, 2020

camilamacedo86 commented Apr 2, 2020 • edited Loading

mengqiy commented Apr 2, 2020

camilamacedo86 commented Apr 2, 2020 • edited Loading

mengqiy commented Apr 2, 2020

camilamacedo86 commented Apr 9, 2020

mengqiy Apr 10, 2020

Choose a reason for hiding this comment

camilamacedo86 Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

mengqiy Apr 13, 2020

Choose a reason for hiding this comment

camilamacedo86 Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

mengqiy Apr 13, 2020

Choose a reason for hiding this comment

mengqiy Apr 13, 2020

Choose a reason for hiding this comment

camilamacedo86 Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

mengqiy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 13, 2020

camilamacedo86 commented Apr 1, 2020 •

edited

Loading

camilamacedo86 commented Apr 2, 2020 •

edited

Loading

camilamacedo86 commented Apr 2, 2020 •

edited

Loading

camilamacedo86 Apr 11, 2020 •

edited

Loading

camilamacedo86 Apr 13, 2020 •

edited

Loading

camilamacedo86 Apr 13, 2020 •

edited

Loading