Chore: Update third-party dependencies to their latest releases #1615

nfnt · 2020-07-21T07:56:56Z

What this PR does / why we need it:
This bumps the modules as well as 'golangci-lint' to their latest releases.
It also fixes linter warnings due to updated linters, among them a potential DoS when copying files from archives (gosec G110).
gotest.tools has been removed in favor of stretchr/testify, as it offers the same functionality.

This bump the modules as well as 'golangci-lint' to their latest releases. It also fixes linter warnings due to updated linters, among them a potential DoS when copying files from archives (gosec G110). 'gotest.tools' has been removed in favor of 'stretchr/testify', as it offers the same functionality. Signed-off-by: Jan Schlicht <jan@d2iq.com>

ANeumann82

Looks pretty good, apart from that one change that I commented. It may be ok to change the behavior, but i'd like to get some more opinions about it

pkg/engine/task/podexec/pod_exec.go

pkg/kudoctl/packages/writer/writer_test.go

Signed-off-by: Jan Schlicht <jan@d2iq.com>

alenkacz

Looks good, I have two questions but they probably won't be a blockers. Thanks 👏

alenkacz · 2020-07-22T12:50:39Z

.golangci.yml

@@ -29,5 +29,4 @@ linters-settings:
  dupl:
    threshold: 400
  goimports:
-    # Don't use 'github.com/kudobuilder/kudo', it'll result in unreliable output!
-    local-prefixes: github.com/kudobuilder
+    local-prefixes: github.com/kudobuilder/kudo


what changed here? that we can do it now...

No idea 😆
I had a lot of issues with that back when we introduced golangci-lint. Now I don't see them anymore and have been using it with full prefixes successfully in other (albeit smaller) projects -- kitt and test-tools.

why don't we leave it local-prefixes: github.com/kudobuilder? It seems to function and allows for a usable configuration across projects.

Because that wouldn't just group kudo packages but packages from other projects in the kudobuilder org as well. For example in pkg/engine/task/task_kudo_operator_test.go, a kuttl import was grouped with kudo imports. This will be fixed with this change to local-prefixes.

pkg/engine/renderer/dependencies_test.go

kensipe

make generate leaves the repo dirty
CRDs and bindata.go are dirty

kensipe · 2020-07-22T21:10:28Z

.golangci.yml

@@ -29,5 +29,4 @@ linters-settings:
  dupl:
    threshold: 400
  goimports:
-    # Don't use 'github.com/kudobuilder/kudo', it'll result in unreliable output!
-    local-prefixes: github.com/kudobuilder
+    local-prefixes: github.com/kudobuilder/kudo


why don't we leave it local-prefixes: github.com/kudobuilder? It seems to function and allows for a usable configuration across projects.

kensipe · 2020-07-22T21:26:58Z

I would push the updated changes... but it likely needs some analysis... some of the change looks like simple line limits which result in wrapping... like:

                   email:
-                    description: Email is an optional email address to contact the named maintainer.
+                    description: Email is an optional email address to contact the
+                      named maintainer.
                     type: string

It would be good to understand what is causing this change... and if innocuous.. then including the changed files

kensipe · 2020-07-22T21:33:53Z

after generating files make generate, make test passes... but make integration-test fails. (around crds)

kensipe · 2020-07-22T21:46:37Z

additionally... golangci needs a change to install-golangcilint.sh which is missing in this change.
https://github.com/kudobuilder/kudo/blob/main/hack/install-golangcilint.sh#L7

which makes me question if the correct linter version was used..

kensipe · 2020-07-22T21:51:39Z

I should add.. there are some really good mods here... In particular the error handling around io.Copy and size limitations!

nfnt · 2020-07-23T06:48:19Z

@kensipe, interesting,make generate was run here and resulted in updated CRDs, make integration-test succeeds in the CI. But controller-gen isn't being updated as part of this PR. This must be due to the Kubernetes APIs bump to 0.18.6. The updated APIs are used by controller-gen as well -- because that's how dependency resolution works with Go modules. As a result, the output is different.

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt · 2020-07-23T06:56:25Z

This would also explain why running make generate locally leaves your repo dirty: You already have controller-gen in the right version installed. But if you would rebuild controller-gen, it would use different dependencies, which results in a different binary.
This is a problem 😦. Thanks for surfacing this.

Signed-off-by: Jan Schlicht <jan@d2iq.com>

kensipe

lgtm

It was necessary to delete controller-gen and go-bindata. after that all is well!

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt requested review from alenkacz, gerred, kensipe and zen-dog as code owners July 21, 2020 07:56

nfnt added component/linter component/testing dependencies Pull requests that update a dependency file labels Jul 21, 2020

nfnt changed the title ~~Update third-party dependencies to their latest releases~~ Chore: Update third-party dependencies to their latest releases Jul 21, 2020

nfnt requested a review from ANeumann82 July 21, 2020 08:06

ANeumann82 requested changes Jul 21, 2020

View reviewed changes

pkg/engine/task/podexec/pod_exec.go Outdated Show resolved Hide resolved

pkg/kudoctl/packages/writer/writer_test.go Outdated Show resolved Hide resolved

Different approach to mitigate decompression bombs

83f1c6d

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt requested a review from ANeumann82 July 21, 2020 12:51

alenkacz approved these changes Jul 22, 2020

View reviewed changes

kensipe requested changes Jul 22, 2020

View reviewed changes

Bump 'golangci-lint' in script

74ce5a9

Signed-off-by: Jan Schlicht <jan@d2iq.com>

Bump 'go-cmp'

e590da0

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt requested a review from kensipe July 23, 2020 08:01

kensipe approved these changes Jul 23, 2020

View reviewed changes

ANeumann82 approved these changes Jul 23, 2020

View reviewed changes

Update 'go-bindata' to v3.1.3

7a6950c

Signed-off-by: Jan Schlicht <jan@d2iq.com>

nfnt merged commit 8b3711c into main Jul 23, 2020

nfnt deleted the nfnt/bump-deps branch July 23, 2020 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Update third-party dependencies to their latest releases #1615

Chore: Update third-party dependencies to their latest releases #1615

nfnt commented Jul 21, 2020

ANeumann82 left a comment

alenkacz left a comment

alenkacz Jul 22, 2020

nfnt Jul 22, 2020

kensipe Jul 22, 2020

nfnt Jul 23, 2020

kensipe left a comment

kensipe Jul 22, 2020

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

nfnt commented Jul 23, 2020

nfnt commented Jul 23, 2020

kensipe left a comment

Chore: Update third-party dependencies to their latest releases #1615

Chore: Update third-party dependencies to their latest releases #1615

Conversation

nfnt commented Jul 21, 2020

ANeumann82 left a comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz Jul 22, 2020

Choose a reason for hiding this comment

nfnt Jul 22, 2020

Choose a reason for hiding this comment

kensipe Jul 22, 2020

Choose a reason for hiding this comment

nfnt Jul 23, 2020

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Jul 22, 2020

Choose a reason for hiding this comment

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

kensipe commented Jul 22, 2020

nfnt commented Jul 23, 2020

nfnt commented Jul 23, 2020

kensipe left a comment

Choose a reason for hiding this comment