Added logs to agent backend and refactored manager backend #665
Conversation
Fixed small documentation issues in CONTRIBUTING.md and USAGE.md Added logs to server.go
Added logs to spire_api.go for agent backend
NordLee
requested review from
lumjjb,
maia-iyer,
mamy-CS and
mrsabath
as code owners
| ) | ||
|
|
||
| func (s *Server) healthcheck(w http.ResponseWriter, r *http.Request) { | ||
| log.Printf("LOG is working") |
There was a problem hiding this comment.
Was this added unintentionally?
| serverConfig := s.TornjakConfig.Server | ||
| if serverConfig.HTTPConfig == nil { | ||
| err = fmt.Errorf("HTTP Config error: no port configured") | ||
| log.Println("Error:", err) // <-- New log |
There was a problem hiding this comment.
It seems to me that the previous error message was more descriptive
There was a problem hiding this comment.
I also don't think it's necessary to add // <-- New log to all the new comments
| if serverConfig.HTTPSConfig.ListenPort == 0 { | ||
| // Fail because this is required field in this section | ||
| err = fmt.Errorf("HTTPS Config error: no port configured. Starting insecure HTTP connection at %d", serverConfig.HTTPConfig.ListenPort) | ||
| log.Println("Error:", err) // <-- New log |
There was a problem hiding this comment.
Similar comment, the previous error seems to be more descriptive
There was a problem hiding this comment.
What are these new files for?
There was a problem hiding this comment.
I see a number of them including =, CACHED, and ERROR
| err = fmt.Errorf("server error serving on https: %w", err) | ||
| log.Println("HTTPS server error:", err) // <-- New log |
There was a problem hiding this comment.
I think these two lines can be combined
| fmt.Printf("Starting to listen on %s...\n", addr) | ||
| log.Printf("Starting HTTP server on %s...\n", addr) // <-- New log |
There was a problem hiding this comment.
These can also be combined
| type HealthcheckResponse grpc_health_v1.HealthCheckResponse | ||
|
|
||
| func (s *Server) SPIREHealthcheck(inp HealthcheckRequest) (*HealthcheckResponse, error) { //nolint:govet //Ignoring mutex (not being used) - sync.Mutex by value is unused for linter govet | ||
| log.Println("SPIREHealthcheck: Starting health check for SPIRE server") |
There was a problem hiding this comment.
I'm not sure if it's necessary to add SPIREHealthcheck to the beginning of all of these error messages. We can go with this for now.
There was a problem hiding this comment.
Where is this manifest being used?
|
Can you describe on a high level your intentions behind the refactoring the manager backend? If I had a better understanding of your strategy, I would have an easier time reviewing your PR |
2 participants
minor changes to to documentation is just fixing invalid urls