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 dependencies #8431

Merged

Conversation

ericzzzzzzz
Copy link
Contributor

@ericzzzzzzz ericzzzzzzz commented Feb 12, 2023

Fixes: #8418

Description:

  • The purpose of this pr is to upgrade some vulnerable dependencies reported by dependency bot. The upgrade of those caused their prerequisites upgraded as well which led to some broken code. Actions are taken for fixing those:
    • open telemetry migration, mainly in the instrument package
    • goyaml upgrade causes that marsharlling array has different format from before, the current one has indentation before -, the previous one was aligned with its parent. Our integration tests use a lot of hard code yaml raw string comparison, the is not a good practice, we should compare the target unmarshalled object field instead, to not include a lot of changes in this pr, I only convert those raw string into object for comparison, but we can make it better.
    • go-git doesn't support adding empty commit by default anymore, hence specifying it in the test config.
    • fix ko-tests
    • deleted code related to generate pipeline, it was broken after upgrade dependencies and I didn't see we use this anywhere, let me know if this is still in use.

@ericzzzzzzz ericzzzzzzz marked this pull request as draft February 12, 2023 21:25
},
sdktrace.WithSampler(sdktrace.AlwaysSample()),
)
exporter, err := texporter.New()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method has already handled default credential fetching.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #8431 (daabddc) into main (290280e) will decrease coverage by 5.30%.
The diff coverage is 55.02%.

@@            Coverage Diff             @@
##             main    #8431      +/-   ##
==========================================
- Coverage   70.48%   65.18%   -5.30%     
==========================================
  Files         515      603      +88     
  Lines       23150    29902    +6752     
==========================================
+ Hits        16317    19492    +3175     
- Misses       5776     8934    +3158     
- Partials     1057     1476     +419     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 426 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aaron-prindle
Copy link
Contributor

Linter tests failing related to gci:

pkg/skaffold/render/renderer/kubectl/kubectl_test.go:22: File is not `gci`-ed with --skip-generated -s standard,default,prefix(github.com/GoogleContainerTools/skaffold) (gci)
	"github.com/google/go-cmp/cmp"

You can download gci here:
https://github.com/daixiang0/gci

This can fixed running something like:

$ gci print -s "standard,default,prefix(github.com/GoogleContainerTools/skaffold)" pkg/skaffold/render/renderer/kubectl/kubectl_test.go

@ericzzzzzzz ericzzzzzzz marked this pull request as ready for review February 13, 2023 04:34
@ericzzzzzzz
Copy link
Contributor Author

Linter tests failing related to gci:

pkg/skaffold/render/renderer/kubectl/kubectl_test.go:22: File is not `gci`-ed with --skip-generated -s standard,default,prefix(github.com/GoogleContainerTools/skaffold) (gci)
	"github.com/google/go-cmp/cmp"

You can download gci here: https://github.com/daixiang0/gci

This can fixed running something like:

$ gci print -s "standard,default,prefix(github.com/GoogleContainerTools/skaffold)" pkg/skaffold/render/renderer/kubectl/kubectl_test.go

Thank you! I usually run make linters locally and then fixing the issue, but this command seems broken on latest macOS, I'll try gci next time.

if traceEnabled {
return trace.WithAttributes(attribute.KeyValue{Key: "error", Value: attribute.StringValue(PII(err.Error()))})
return trace.WithStackTrace(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now span.End takes SpanEndOption as input, however, we cannot create a SpanEndOption WithAttribute, I'd prefer to leave the code here, we can revisit that when working on exporting metrics to firelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM!

@aaron-prindle aaron-prindle merged commit e1bd270 into GoogleContainerTools:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update skaffold go version and related deps to meet security posture
2 participants