gruntwork-io / health-checker

A simple HTTP server that will return 200 OK if all configured health checks pass.
MIT License
103 stars 37 forks source link

Running health-checker with HEALTH_CHECKER_DEBUG="true" fails to start #8

Open sarkis opened 6 years ago

sarkis commented 6 years ago

$ HEALTH_CHECKER_DEBUG="true" health-checker --port 5500 INFO[2018-02-25T12:35:02-08:00] Note: To enable debug mode, set HEALTH_CHECKER_DEBUG to "true"

digging further looks like it is: https://github.com/gruntwork-io/health-checker/blob/f909446f280d7a0bbf0537d5db66736e93de9bed/commands/cli.go#L52-L53

Is there some reason we want this to exit the app here or is this a bug?

brikis98 commented 6 years ago

/cc @josh-padnick

josh-padnick commented 6 years ago

Ah, this is a bug. My thought here was: If there's an error and we're NOT in Debug mode, let the user know they can enable debug mode for additional logging. So this should be:

if err != nil && ! isDebugMode() {
    opts.Logger.Infof("Note: To enable debug mode, set %s to \"true\"", ENV_VAR_NAME_DEBUG_MODE)
    return err
}

Update: Also, you may want to make the if statement below that an else if err != nil.

sarkis commented 6 years ago

I hit another little snag here - worth noting I think as I was scratching my head on why I was getting a pointer error:

opts, err := parseOptions(cliContext)
if err != nil && ! isDebugMode() {
    opts.Logger.Infof("Note: To enable debug mode, set %s to \"true\"", ENV_VAR_NAME_DEBUG_MODE)
    return err
}

Finally realized can't call opts.Logger inside of that conditional, since it would/should be nil if the conditional if err != nil evaluated to true... :headdesk:

Fixed this in #9 - for now I dropped the Note message - but can see how we can initialize a logger if you think it's worth adding the note.

josh-padnick commented 6 years ago

Ah, that makes sense. When I was writing that line, I tested it in a way where parseOptions() didn't actually throw an error and did finish processing correctly. Removing the note message sounds fine for now.