Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(*) upgrade github.com/spf13/cobra #2732

Merged
merged 2 commits into from
Sep 7, 2021
Merged

chore(*) upgrade github.com/spf13/cobra #2732

merged 2 commits into from
Sep 7, 2021

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Sep 6, 2021

Summary

Upgrade github.com/spf13/cobra.

Full changelog

N/A

Issues resolved

N/A

Documentation

N/A

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

Signed-off-by: James Peach <james.peach@konghq.com>
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! LGTM though there seems to be something broken as a result of this...
Seems like when we're switching to the builtin method we should probably get rid of the golden files for completion (IMO reading this kind of code on review as little value other than review fatigue).

@jpeach
Copy link
Contributor Author

jpeach commented Sep 6, 2021

Thanks for doing this! LGTM though there seems to be something broken as a result of this...
Seems like when we're switching to the builtin method we should probably get rid of the golden files for completion (IMO reading this kind of code on review as little value other than review fatigue).

There's no code changes here, so the changes to the golden files are just changes in whatever cobra generates. I agree that there's little value in comparing that to anything.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 6, 2021

This is weird, ginkgo now fails without any discernible test failures. Pretty sure that it's failing in the transparent proxy installation tests and the error is swallowed because of #2625.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 6, 2021

This is weird, ginkgo now fails without any discernible test failures. Pretty sure that it's failing in the transparent proxy installation tests and the error is swallowed because of #2625.

Here's the problem:

(dlv) bt
 0  0x00000000004f73ef in os.Exit
    at /usr/lib/go-1.16/src/os/proc.go:62
 1  0x0000000002106585 in github.com/kumahq/kuma/pkg/transparentproxy/istio/tools/istio-clean-iptables/pkg/cmd.handleError
    at ./pkg/transparentproxy/istio/tools/istio-clean-iptables/pkg/cmd/root.go:172
 2  0x000000000210615c in github.com/kumahq/kuma/pkg/transparentproxy/istio/tools/istio-clean-iptables/pkg/cmd.bindFlags
    at ./pkg/transparentproxy/istio/tools/istio-clean-iptables/pkg/cmd/root.go:136
 3  0x00000000006f9d24 in github.com/spf13/cobra.(*Command).execute
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:849
 4  0x00000000006fa905 in github.com/spf13/cobra.(*Command).ExecuteC
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:974
 5  0x00000000006fa03b in github.com/spf13/cobra.(*Command).Execute
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:902
 6  0x0000000002107545 in github.com/kumahq/kuma/pkg/transparentproxy/istio.(*IstioTransparentProxy).Cleanup
    at ./pkg/transparentproxy/istio/istio.go:87
 7  0x0000000002198f75 in github.com/kumahq/kuma/app/kumactl/cmd/install.modifyIpTables
    at ./app/kumactl/cmd/install/install_transparent_proxy.go:205
 8  0x00000000021a3a96 in github.com/kumahq/kuma/app/kumactl/cmd/install.newInstallTransparentProxy.func1
    at ./app/kumactl/cmd/install/install_transparent_proxy.go:145
 9  0x000000000222e7c2 in github.com/kumahq/kuma/app/kumactl/pkg/errors.FormatErrorWrapper.func1
    at ./app/kumactl/pkg/errors/formatter.go:14
10  0x00000000006f99b6 in github.com/spf13/cobra.(*Command).execute
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:856
11  0x00000000006fa905 in github.com/spf13/cobra.(*Command).ExecuteC
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:974
12  0x00000000006fa03b in github.com/spf13/cobra.(*Command).Execute
    at /home/jpeach/go/pkg/mod/github.com/spf13/cobra@v1.2.1/command.go:902
13  0x00000000029f9b95 in github.com/kumahq/kuma/app/kumactl/cmd/install_test.glob..func8.2
    at ./app/kumactl/cmd/install/install_transparent_proxy_test.go:44
> os.Exit() /usr/lib/go-1.16/src/os/proc.go:62 (hits goroutine(35):1 total:1) (PC: 0x4f73ef)
Frame 1: ./pkg/transparentproxy/istio/tools/istio-clean-iptables/pkg/cmd/root.go:172 (PC: 2106585)
   167:		}
   168:	}
   169:	
   170:	func handleError(err error) {
   171:		fmt.Printf("%v\n", err)
=> 172:		os.Exit(1)
   173:	}
(dlv) p err
error(*errors.errorString) *{
	s: "flag for \"dns-upstream-target-chain\" is nil",}

So the DNSUpstreamTargetChain viper binding introduced in #1821 didn't have a corresponding flag, and until the upgrade, viper seems to have ignored it rather than puke an error. Because of the shitty error described in #2625, this packages just exits without emitting any errors.

Signed-off-by: James Peach <james.peach@konghq.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2732 (1e9f063) into master (80c63ad) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2732      +/-   ##
==========================================
- Coverage   52.10%   52.09%   -0.02%     
==========================================
  Files         866      866              
  Lines       49475    49478       +3     
==========================================
- Hits        25778    25774       -4     
- Misses      21609    21624      +15     
+ Partials     2088     2080       -8     
Impacted Files Coverage Δ
...ntproxy/istio/tools/istio-iptables/pkg/cmd/root.go 56.52% <50.00%> (-0.06%) ⬇️
...y/istio/tools/istio-clean-iptables/pkg/cmd/root.go 63.51% <100.00%> (+0.49%) ⬆️
pkg/insights/components.go 70.00% <0.00%> (-30.00%) ⬇️
.../core/managers/apis/ratelimit/ratelimit_manager.go 35.48% <0.00%> (-9.68%) ⬇️
pkg/events/eventbus.go 85.18% <0.00%> (-7.41%) ⬇️
pkg/insights/resyncer.go 64.17% <0.00%> (-2.99%) ⬇️
pkg/core/resources/manager/cache.go 84.41% <0.00%> (+2.59%) ⬆️
app/kumactl/cmd/root.go 73.68% <0.00%> (+3.50%) ⬆️
pkg/core/bootstrap/autoconfig.go 54.46% <0.00%> (+8.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80c63ad...1e9f063. Read the comment docs.

@jpeach jpeach merged commit 56881c1 into kumahq:master Sep 7, 2021
@jpeach jpeach deleted the chore/upgrade-cobra branch September 7, 2021 08:02
mergify bot pushed a commit that referenced this pull request Sep 7, 2021
Signed-off-by: James Peach <james.peach@konghq.com>
(cherry picked from commit 56881c1)
jpeach added a commit that referenced this pull request Sep 8, 2021
Signed-off-by: James Peach <james.peach@konghq.com>
(cherry picked from commit 56881c1)

Co-authored-by: James Peach <james.peach@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
Signed-off-by: James Peach <james.peach@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
Signed-off-by: James Peach <james.peach@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants