checkshttp #230
checkshttp #230
Conversation
jesusfcr
commented
Dec 4, 2024
•
jesusfcr
commented
- Accept requests from http
- Add NewCheckLogFromContext func
- Upgrade dependencies
- Fix lints
- Allow env variables in command exec helpers
- Migrate from travis to GH actions
jesusfcr
force-pushed
the
checkshttp
branch
5 times, most recently
from
82a6f10 to
7913b74
Compare
jesusfcr
force-pushed
the
checkshttp
branch
5 times, most recently
from
1fabcb4 to
48daaf8
Compare
config/config.go
Outdated
| port := os.Getenv(httpPort) | ||
| if port != "" { | ||
| p, err := strconv.Atoi(port) | ||
| if err == nil { | ||
| c.Port = &p | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd change the signature of the function to return an error if the port is not valid.
| port := os.Getenv(httpPort) | |
| if port != "" { | |
| p, err := strconv.Atoi(port) | |
| if err == nil { | |
| c.Port = &p | |
| } | |
| } | |
| port := os.Getenv(httpPort) | |
| if port != "" { | |
| if p, err := strconv.Atoi(port); err != nil { | |
| return fmt.Errorf("invalid port number: %v", port) | |
| } | |
| c.Port = &p | |
| } |
There was a problem hiding this comment.
This API is a mess. There are too many functions that do the same with small variations and very verbose names. It should be rewritten. But, of course, not in this PR.
There was a problem hiding this comment.
Not only that, IMHO they should be in an internal package inside vulcan-checks. See adevinta/vulcan-checks#786
internal/http/check.go
Outdated
| // shutting down the http server and waiting | ||
| func (c *Check) Shutdown() error { | ||
| // Send the exit signal to shutdown the server. | ||
| c.exitSignal <- syscall.SIGTERM |
There was a problem hiding this comment.
I would use different channels for the signals and the user shutdown order.
internal/http/check.go
Outdated
| // Send the exit signal to shutdown the server. | ||
| c.exitSignal <- syscall.SIGTERM | ||
|
|
||
| c.shuttedDown = make(chan int) |
There was a problem hiding this comment.
I'd initialize the channel on the constructor and maybe rename it to done and change its type to chan bool.
internal/http/check.go
Outdated
| if c.shuttedDown != nil { | ||
| c.Logger.Info("Notifying shuttedDown") | ||
| c.shuttedDown <- 1 | ||
| c.Logger.Info("Notified shuttedDown") | ||
| } |
There was a problem hiding this comment.
With the proposed changes I think this can be replaced with:
| if c.shuttedDown != nil { | |
| c.Logger.Info("Notifying shuttedDown") | |
| c.shuttedDown <- 1 | |
| c.Logger.Info("Notified shuttedDown") | |
| } | |
| c.Logger.Info("Notifying shutdown") | |
| c.done <- true | |
| c.Logger.Info("Notified shutdown") |
internal/http/check.go
Outdated
| } | ||
| }() | ||
|
|
||
| s := <-c.exitSignal |
There was a problem hiding this comment.
I would replace it with a select that checks both the signals and the user channels.
|
|
||
| // ServeHTTP implements an HTTP POST handler that receives a JSON encoded job, and returns an | ||
| // agent.State JSON encoded response. | ||
| func (c *Check) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
I would not call this function ServeHttp. It is like Check implements http.Handler but that's not the real purpose.
