-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add app-entrypoint command #48
Conversation
be6d810
to
49ccc3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. All of the comments are nits
t.Logf("Check the fake app process is still running") | ||
proc, err := os.FindProcess(appPid) | ||
require.NoError(t, err, "Failed to find fake app process") | ||
// A zero-signal lets us check the process is still valid/running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised there isn't a better way to accomplish this, but this is fantastic
Co-authored-by: Eric Haberkorn <erichaberkorn@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is brilliant work!
return exitCode | ||
} | ||
if c.shutdownDelay > 0 { | ||
c.log.Info(fmt.Sprintf("consul-ecs: received sigterm. waiting %s before terminating application.", c.shutdownDelay)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we include the datetime here or use hclog? Might be useful in any debugging situaton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using hclog, so we get a timestamp:
2021-11-30T10:49:05.150-0600 [INFO] consul-ecs: received sigterm. waiting 5s before terminating application.
(Also, CloudWatch has its own timestamps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, then can we use k/v logging instead of fmt.Sprintf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for this log fmt.Sprintf reads better
Changes proposed in this PR:
Add
app-entrypoint
subcommand, which effectively supports delaying the TERM signal on Task shutdown for a certain amount of time.How I've tested this PR:
How I expect reviewers to test this PR:
馃憖
Checklist: