spiffe / spire

The SPIFFE Runtime Environment
https://spiffe.io
Apache License 2.0
1.79k stars 472 forks source link

Enforce desired casing for logs and error messages #835

Closed amoore877 closed 3 years ago

amoore877 commented 5 years ago

Sub-issue of #799

It may be prudent to decide on a standard for casing on log messages and on custom error messages (which may be different from each other). This standard should then be documented in CONTRIBUTING.md and enforced in PRs.

This may be a medium to large undertaking, as it may be decided that current messages should all be updated to meet the standards. It may then be appropriate to break this down into further sub-issues by file or group of files.

amoore877 commented 5 years ago

General standard is that errors should start as lower case, and logged messages should follow standard casing.

evan2645 commented 5 years ago

General standard is that errors should start as lower case, and logged messages should follow standard casing.

+1. A small comment in CONTRIBUTING.md in addition to a linter would be awesome. Perhaps the former is even optional so long as "run the linter" is specified in there somewhere.

azdagron commented 3 years ago

I think we've made good progress here. It would probably be a pretty quick win to just update all of the existing log lines that don't follow the convention. There aren't too many:

$ ag '[lL]og.*\.(Debug|Warn|Info|Error)[f]?\("[a-z]' -l
cmd/spire-agent/cli/run/run.go
cmd/spire-server/cli/run/run.go
pkg/server/plugin/datastore/sql/migration.go
pkg/server/plugin/datastore/sql/sqlite.go
pkg/server/plugin/upstreamauthority/vault/vault_client.go
pkg/server/endpoints/bundle/server.go
pkg/server/endpoints/node/limiter.go
pkg/server/endpoints/node/handler.go
pkg/server/server.go
pkg/server/ca/manager.go
pkg/common/log/log_test.go
pkg/common/catalog/builtin.go
pkg/common/catalog/external.go
pkg/common/jwtutil/keyset.go
pkg/common/hostservices/metricsservice/plugin_metrics.go
pkg/agent/agent.go
pkg/agent/plugin/workloadattestor/k8s/k8s.go
pkg/common/health/health.go
pkg/agent/manager/cache/cache.go
pkg/agent/manager/manager.go
pkg/agent/client/client.go
pkg/agent/attestor/node/experimental.go
SilvaMatteus commented 3 years ago

Hello folks. I found no guidelines about closing punctuation. Do you have any directions on this?

SilvaMatteus commented 3 years ago

Also, the log message

cmd/spire-server/cli/run/run.go
453:                    sc.Log.Warn("ca_subject configurable is set but empty; the default will be used")

should remain unchanged, isn't it?

amartinezfayo commented 3 years ago

Thanks @SilvaMatteus for working on this!

I found no guidelines about closing punctuation. Do you have any directions on this?

We don't use periods to end sentences in logs and error messages. I think that it would be good to explicit that in the Logs and Errors section of the contributing guide.

sc.Log.Warn("ca_subject configurable is set but empty; the default will be used") should remain unchanged, isn't it?

Yes, I think that it should remain as is since the start of the sentence is referring to a setting name.