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

feat: introduce slog for zarf tools #3212

Merged
merged 11 commits into from
Nov 21, 2024
Merged

feat: introduce slog for zarf tools #3212

merged 11 commits into from
Nov 21, 2024

Conversation

AustinAbro321
Copy link
Contributor

@AustinAbro321 AustinAbro321 commented Nov 7, 2024

Description

Introduces slog for zarf tools. Because of the complexity around cmd, I decided to make the crane commands use the default logger, as right now they don't receive flags properly. This should be fixed when cmd is refactored.

Additionally the largest change here is around zarf tools update-creds which has a very unique output at the moment. Here are the changes side by side
image
image

Related Issues

Relates to #2576

Checklist before merging

Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 70dd782
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/673cfe2432315f0008ce2fa9

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 3.38983% with 114 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/cmd/tools/zarf.go 0.00% 67 Missing ⚠️
src/cmd/tools/crane.go 0.00% 42 Missing ⚠️
src/internal/packager/helm/chart.go 0.00% 2 Missing ⚠️
src/cmd/root.go 0.00% 1 Missing ⚠️
src/cmd/tools/helm.go 0.00% 1 Missing ⚠️
src/cmd/tools/kubectl.go 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/pkg/cluster/secrets.go 64.34% <100.00%> (+1.28%) ⬆️
src/cmd/root.go 0.00% <0.00%> (ø)
src/cmd/tools/helm.go 0.00% <0.00%> (ø)
src/cmd/tools/kubectl.go 0.00% <0.00%> (ø)
src/internal/packager/helm/chart.go 12.33% <0.00%> (-0.07%) ⬇️
src/cmd/tools/crane.go 0.00% <0.00%> (ø)
src/cmd/tools/zarf.go 0.00% <0.00%> (ø)

🚨 Try these New Features:

Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 marked this pull request as ready for review November 19, 2024 19:12
@AustinAbro321 AustinAbro321 requested review from a team as code owners November 19, 2024 19:12
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 changed the title feat: introduce slog for zarf tools feat: introduce slog for zarf tools Nov 19, 2024
Signed-off-by: Austin Abro <[email protected]>
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

Most of the changes look good but I have reservations about coupling logger into message. Suggested an alternative.

src/pkg/message/credentials.go Outdated Show resolved Hide resolved
src/pkg/message/credentials.go Outdated Show resolved Hide resolved
src/cmd/tools/zarf.go Fixed Show fixed Hide fixed
src/cmd/tools/zarf.go Fixed Show fixed Hide fixed
src/cmd/tools/zarf.go Fixed Show fixed Hide fixed
src/cmd/tools/zarf.go Fixed Show fixed Hide fixed
Signed-off-by: Austin Abro <[email protected]>
Copy link
Contributor

@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

LGTM

@AustinAbro321 AustinAbro321 added this pull request to the merge queue Nov 21, 2024
Merged via the queue into main with commit 837f2ca Nov 21, 2024
26 checks passed
@AustinAbro321 AustinAbro321 deleted the slog-tools branch November 21, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants