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.

Sign up for GitHub

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

Jump to bottom

deps: convert zap to slog #2825

Merged
merged 54 commits into from
Feb 8, 2024
Merged

deps: convert zap to slog #2825

merged 54 commits into from
Feb 8, 2024

Conversation

miampf
Copy link
Contributor

@miampf miampf commented Jan 15, 2024

Context

Rewrite packages to use slog instead of zap.

Proposed change(s)

  • change occurences of zap to slog
  • don't use our own logger struct anymore; slog should be used directly
  • reduce the logger package to a minimum of functionality often used together with slog

Additional info

  • operators was not rewritten
  • Since slog has no Fatal() function it was replaced by a call to Error() followed by os.Exit(1)
    • because of that logs will contain level=ERROR instead of level=FATAL
    • This can be fixed with an replaceAttr() function that could be written in the logger package, however this would cause more boilerplate in the logger package and accross the whole project, so I decided against it

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Link to Milestone

@miampf miampf added no changelog Change won't be listed in release changelog dependency labels Jan 15, 2024
@miampf miampf added this to the v2.15.0 milestone Jan 15, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 15, 2024 •
edited
Loading

✅ Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 9cdc343
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/65a55883baa94a0008414bbf

@netlify Netlify
Copy link

netlify bot commented Jan 15, 2024 •
edited
Loading

✅ Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit aa6d14e
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/65c4cbfd33c59b0008726d79

msanft
msanft requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

Went over internal/logger, dev-docs/conventions.md, internal/attestation, joinservice, terraform-provider-constellation, internal/constellation. Mostly some usability nit-picks. Overall structure looks good to me!

dev-docs/conventions.md Outdated Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
internal/logger/log.go Show resolved Hide resolved
internal/attestation/aws/snp/validator_test.go Outdated Show resolved Hide resolved
internal/attestation/azure/snp/validator.go Show resolved Hide resolved
joinservice/cmd/main.go Outdated Show resolved Hide resolved
terraform-provider-constellation/internal/provider/cluster_resource.go Show resolved Hide resolved
daniel-weisse
daniel-weisse reviewed Jan 16, 2024
View reviewed changes
dev-docs/conventions.md Outdated Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
elchead
elchead approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@elchead elchead left a comment

Choose a reason for hiding this comment

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

there we no relevant changes in my CODEOWNER files except for the provider. lgtm

thomasten
thomasten requested changes Jan 16, 2024
View reviewed changes
dev-docs/conventions.md Outdated Show resolved Hide resolved
dev-docs/conventions.md Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
internal/logger/grpclogger.go Outdated Show resolved Hide resolved
internal/grpc/grpclog/grpclog.go Outdated Show resolved Hide resolved
internal/grpc/grpclog/grpclog_test.go Outdated Show resolved Hide resolved
@miampf miampf force-pushed the zap-to-slog branch 5 times, most recently from e673aa7 to 2e56702 Compare January 24, 2024 14:31
@burgerdev burgerdev modified the milestones: v2.15.0, v2.16.0 Jan 26, 2024
thomasten
thomasten requested changes Jan 29, 2024
View reviewed changes
dev-docs/conventions.md Outdated Show resolved Hide resolved
internal/logger/cmdline.go Outdated Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
internal/logger/log.go Outdated Show resolved Hide resolved
terraform-provider-constellation/internal/provider/cluster_resource.go Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/log.go Outdated Show resolved Hide resolved
@miampf miampf force-pushed the zap-to-slog branch 5 times, most recently from cf7493c to 5132017 Compare January 29, 2024 15:51
miampf and others added 21 commits February 8, 2024 13:15
@miampf
added copyright notice
d80383d
@malt3 @miampf
tidy
972f832
@miampf
fixed issues detected by bazel check
081d357
@miampf
updated conventions.md to more idiomatic suggestion
8a246a0
@miampf
fixed issues from rebase
f875466
@miampf
log with LevelError if unknown level is used for middlewareLogger
62f2e44
@miampf
rewrote use of With() in dev-docs
747af57
@miampf
Verbosity numbers now correspond to slog log Levels
e94a04c
@miampf
clean up the middleware logger with r.Add()
8204e06
@miampf
rewrote apply.go warnLog/warnLogger
4854e97
@miampf
replaced format with msg in needed parameters
c8b0175
@miampf
bazel tidy
f8a3d65
@miampf
fixed usage of Debugf due to rebase
e14f767
@miampf
go back to old verbosity conversion
f8a1d92
@miampf
added comments to avoid confusion
f7d2e1c
@miampf
add period to comments
1231785
@miampf
rewrote every slog logger to add source
5d6d3ab
@miampf
added new point in conventions.md to use provided helper functions
e435211
@miampf
also add source to Test logger
b0bbea8
@miampf
gazelle_generate + tidy
cc980b5
@miampf
fixed issues from last rebase
2cd44b2
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

Coverage report

Package Old New Trend
bootstrapper/cmd/bootstrapper [no test files] [no test files] 🚧
bootstrapper/internal/initserver 73.70% 73.70% ↔️
bootstrapper/internal/joinclient 88.40% 88.40% ↔️
bootstrapper/internal/kubernetes 74.80% 74.80% ↔️
bootstrapper/internal/kubernetes/k8sapi 13.40% 13.40% 🚧
cli/internal/cmd 58.50% 58.80% ↗️
debugd/cmd/debugd [no test files] [no test files] 🚧
debugd/internal/cdbg/cmd [no test files] [no test files] 🚧
debugd/internal/debugd/deploy 83.60% 83.60% ↔️
debugd/internal/debugd/logcollector 13.70% 13.70% 🚧
debugd/internal/debugd/metadata 94.10% 94.10% ↔️
debugd/internal/debugd/server 70.70% 69.50% ↘️
debugd/internal/filetransfer 100.00% 100.00% ↔️
disk-mapper/cmd [no test files] [no test files] 🚧
disk-mapper/internal/diskencryption [no test files] [no test files] 🚧
disk-mapper/internal/recoveryserver 89.10% 89.10% ↔️
disk-mapper/internal/rejoinclient 93.10% 93.10% ↔️
disk-mapper/internal/setup 68.90% 68.90% ↔️
e2e/malicious-join [no test files] [no test files] 🚧
hack/bazel-deps-mirror [no test files] [no test files] 🚧
hack/bazel-deps-mirror/internal/mirror 80.20% 80.20% ↔️
hack/cli-k8s-compatibility [no test files] [no test files] 🚧
hack/oci-pin [no test files] [no test files] 🚧
hack/qemu-metadata-api [no test files] [no test files] 🚧
hack/qemu-metadata-api/server 57.90% 57.90% ↔️
image/upload/internal/cmd [no test files] [no test files] 🚧
internal/api/attestationconfigapi 32.60% 32.60% 🚧
internal/api/attestationconfigapi/cli [no test files] [no test files] 🚧
internal/api/client [no test files] [no test files] 🚧
internal/api/versionsapi 69.80% 69.80% ↔️
internal/api/versionsapi/cli [no test files] [no test files] 🚧
internal/attestation 66.70% 66.70% ↔️
internal/attestation/azure/snp 31.20% 31.20% 🚧
internal/attestation/snp 85.30% 85.30% ↔️
internal/attestation/tdx [no test files] [no test files] 🚧
internal/attestation/vtpm 22.40% 22.40% 🚧
internal/cloud/azure 70.00% 70.00% ↔️
internal/constellation 60.50% 60.50% ↔️
internal/constellation/helm 53.20% 53.20% ↔️
internal/constellation/kubecmd 63.90% 63.90% ↔️
internal/grpc/grpclog 73.30% 73.30% ↔️
internal/logger [no test files] [no test files] 🚧
internal/osimage/archive [no test files] [no test files] 🚧
internal/osimage/imageinfo [no test files] [no test files] 🚧
internal/osimage/measurementsuploader [no test files] [no test files] 🚧
internal/osimage/nop [no test files] [no test files] 🚧
internal/osimage/uplosi [no test files] [no test files] 🚧
internal/staticupload 79.20% 79.20% ↔️
internal/verify 17.10% 17.10% 🚧
joinservice/cmd [no test files] [no test files] 🚧
joinservice/internal/certcache 70.70% 70.70% ↔️
joinservice/internal/certcache/amdkds 100.00% 100.00% ↔️
joinservice/internal/kms 85.70% 85.70% ↔️
joinservice/internal/kubeadm 76.10% 76.10% ↔️
joinservice/internal/kubernetesca 81.60% 81.60% ↔️
joinservice/internal/server 79.20% 79.20% ↔️
joinservice/internal/watcher 73.70% 73.70% ↔️
keyservice/cmd [no test files] [no test files] 🚧
keyservice/internal/server 61.90% 61.90% ↔️
measurement-reader/cmd [no test files] [no test files] 🚧
s3proxy/cmd [no test files] [no test files] 🚧
s3proxy/internal/kms 85.70% 85.70% ↔️
s3proxy/internal/router 4.20% 4.20% 🚧
terraform-provider-constellation/internal/provider 6.60% 6.60% 🚧
upgrade-agent/cmd [no test files] [no test files] 🚧
upgrade-agent/internal/server 20.00% 20.00% 🚧
verify/cmd [no test files] [no test files] 🚧
verify/server 95.40% 95.40% ↔️

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

Coverage report

Package Old New Trend
bootstrapper/cmd/bootstrapper [no test files] [no test files] 🚧
bootstrapper/internal/initserver 73.70% 73.70% ↔️
bootstrapper/internal/joinclient 88.40% 88.40% ↔️
bootstrapper/internal/kubernetes 74.80% 74.80% ↔️
bootstrapper/internal/kubernetes/k8sapi 13.40% 13.40% 🚧
cli/internal/cmd 58.50% 58.80% ↗️
debugd/cmd/debugd [no test files] [no test files] 🚧
debugd/internal/cdbg/cmd [no test files] [no test files] 🚧
debugd/internal/debugd/deploy 83.60% 83.60% ↔️
debugd/internal/debugd/logcollector 13.70% 13.70% 🚧
debugd/internal/debugd/metadata 94.10% 94.10% ↔️
debugd/internal/debugd/server 70.70% 69.50% ↘️
debugd/internal/filetransfer 100.00% 100.00% ↔️
disk-mapper/cmd [no test files] [no test files] 🚧
disk-mapper/internal/diskencryption [no test files] [no test files] 🚧
disk-mapper/internal/recoveryserver 89.10% 89.10% ↔️
disk-mapper/internal/rejoinclient 93.10% 93.10% ↔️
disk-mapper/internal/setup 68.90% 68.90% ↔️
e2e/malicious-join [no test files] [no test files] 🚧
hack/bazel-deps-mirror [no test files] [no test files] 🚧
hack/bazel-deps-mirror/internal/mirror 80.20% 80.20% ↔️
hack/cli-k8s-compatibility [no test files] [no test files] 🚧
hack/oci-pin [no test files] [no test files] 🚧
hack/qemu-metadata-api [no test files] [no test files] 🚧
hack/qemu-metadata-api/server 57.90% 57.90% ↔️
image/upload/internal/cmd [no test files] [no test files] 🚧
internal/api/attestationconfigapi 32.60% 32.60% 🚧
internal/api/attestationconfigapi/cli [no test files] [no test files] 🚧
internal/api/client [no test files] [no test files] 🚧
internal/api/versionsapi 69.80% 69.80% ↔️
internal/api/versionsapi/cli [no test files] [no test files] 🚧
internal/attestation 66.70% 66.70% ↔️
internal/attestation/azure/snp 31.20% 31.20% 🚧
internal/attestation/snp 85.30% 85.30% ↔️
internal/attestation/tdx [no test files] [no test files] 🚧
internal/attestation/vtpm 22.40% 22.40% 🚧
internal/cloud/azure 70.00% 70.00% ↔️
internal/constellation 60.50% 60.50% ↔️
internal/constellation/helm 53.20% 53.20% ↔️
internal/constellation/kubecmd 63.90% 63.90% ↔️
internal/grpc/grpclog 73.30% 73.30% ↔️
internal/logger [no test files] [no test files] 🚧
internal/osimage/archive [no test files] [no test files] 🚧
internal/osimage/imageinfo [no test files] [no test files] 🚧
internal/osimage/measurementsuploader [no test files] [no test files] 🚧
internal/osimage/nop [no test files] [no test files] 🚧
internal/osimage/uplosi [no test files] [no test files] 🚧
internal/staticupload 79.20% 79.20% ↔️
internal/verify 17.10% 17.10% 🚧
joinservice/cmd [no test files] [no test files] 🚧
joinservice/internal/certcache 70.70% 70.70% ↔️
joinservice/internal/certcache/amdkds 100.00% 100.00% ↔️
joinservice/internal/kms 85.70% 85.70% ↔️
joinservice/internal/kubeadm 76.10% 76.10% ↔️
joinservice/internal/kubernetesca 81.60% 81.60% ↔️
joinservice/internal/server 79.20% 79.20% ↔️
joinservice/internal/watcher 73.70% 73.70% ↔️
keyservice/cmd [no test files] [no test files] 🚧
keyservice/internal/server 61.90% 61.90% ↔️
measurement-reader/cmd [no test files] [no test files] 🚧
s3proxy/cmd [no test files] [no test files] 🚧
s3proxy/internal/kms 85.70% 85.70% ↔️
s3proxy/internal/router 4.20% 4.20% 🚧
terraform-provider-constellation/internal/provider 6.60% 6.60% 🚧
upgrade-agent/cmd [no test files] [no test files] 🚧
upgrade-agent/internal/server 20.00% 20.00% 🚧
verify/cmd [no test files] [no test files] 🚧
verify/server 95.40% 95.40% ↔️

@miampf miampf merged commit 54cce77 into main Feb 8, 2024
11 checks passed
@miampf miampf deleted the zap-to-slog branch February 8, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-weisse daniel-weisse daniel-weisse approved these changes

@msanft msanft msanft approved these changes

@thomasten thomasten thomasten approved these changes

@elchead elchead elchead approved these changes

@3u13r 3u13r 3u13r approved these changes

@derpsteb derpsteb derpsteb approved these changes

@malt3 malt3 malt3 approved these changes

Assignees
No one assigned
Labels
dependency no changelog Change won't be listed in release changelog
Projects
None yet
Milestone
v2.16.0
Development

Successfully merging this pull request may close these issues.
9 participants
@miampf @malt3 @derpsteb @3u13r @elchead @thomasten @msanft @daniel-weisse @burgerdev