feat: Gate ECP logs behind ENABLE_ENTERPRISE_CERTIFICATE_LOGS environment variable. #57
Conversation
clundin25
force-pushed
the
env-logging-update
branch
from
eaaa786 to
0901b9f
Compare
|
I am not so familiar with Go so I hope someone can sign off on the module changes. Going to read the golang module documentation now. |
|
Fixes #47 |
go.mod
Outdated
| @@ -1,3 +1,7 @@ | |||
| module github.com/googleapis/enterprise-certificate-proxy | |||
|
|
|||
| go 1.18 | |||
|
|
|||
| replace github.com/googleapis/enterprise-certificate-proxy/utils => ./utils | |||
There was a problem hiding this comment.
I don't think you need this. You just need to make sure the util package is under your local directory: $GOPATH/src/github.com/googleapis/enterprise-certificate-proxy/utils
There was a problem hiding this comment.
Go mod has somewhat replaced GOPATH (and no assumptions should be made on GOPATH as that is very user and env specific). We should not use replace syntax either however.
There was a problem hiding this comment.
Thanks for this PR Carl. Unfortunately, the current iteration will introduce a ton of dependency management challenges that are not easily solvable. I would recommend we defer this PR before launch, or adopt a copy and paste approach if it's important to suppress logging. There are also other ways to implement this, such as having ECP client launch the subprocesses in "silent mode" via an arg at launch, so we only need to process the env var in a single place inside ecp client code.
internal/signer/darwin/signer.go
Outdated
| @@ -20,6 +20,8 @@ import ( | |||
| "signer/keychain" | |||
| "signer/util" | |||
| "time" | |||
|
|
|||
| "github.com/googleapis/enterprise-certificate-proxy/utils" | |||
There was a problem hiding this comment.
Hmm, I don't like introducing this go mod dependency here. For one thing, it will break the copybara sync to google3 (unless we add further copybara transforms in the config) and kokoro transforms. For now, it may be better to duplicate logging util code into the respective signer/util sub directories to avoid this dependency. Go mod and vendoring is excruciating painful to get right, and made worse by google3 import.
internal/signer/darwin/go.mod
Outdated
| go 1.18 | ||
|
|
||
| replace github.com/googleapis/enterprise-certificate-proxy/utils => ../../../utils |
There was a problem hiding this comment.
In general, replace syntax is reserved for local development only and not something to check into the code-base. See my other comment about avoiding this dependency for now. Side-blurb: Golang dependency management is different (in many bad ways) from many other languages. I would think of this repo as 4 independent units: ECP client, ecp darwin, ecp linux, ecp windows with no cross dependency between the 4.
go.mod
Outdated
| @@ -1,3 +1,7 @@ | |||
| module github.com/googleapis/enterprise-certificate-proxy | |||
|
|
|||
| go 1.18 | |||
|
|
|||
| replace github.com/googleapis/enterprise-certificate-proxy/utils => ./utils | |||
There was a problem hiding this comment.
Go mod has somewhat replaced GOPATH (and no assumptions should be made on GOPATH as that is very user and env specific). We should not use replace syntax either however.
|
@andyrzhao I'll go with the copy and paste for now. Adding a ticket to refactor this. I think it is important to avoid polluting the stdout of gcloud. |
clundin25
force-pushed
the
env-logging-update
branch
from
0901b9f to
b969b37
Compare
internal/signer/windows/signer.go
Outdated
| @@ -22,6 +23,17 @@ import ( | |||
| "time" | |||
| ) | |||
|
|
|||
| // / If ECP Logging is enabled return true | |||
There was a problem hiding this comment.
nit: Remove extra slash "/" for all the comments.
No description provided.