Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support StopTimeout for Docker tasks #3601

Merged
merged 3 commits into from
Dec 4, 2017
Merged

Support StopTimeout for Docker tasks #3601

merged 3 commits into from
Dec 4, 2017

Conversation

chelseakomlo
Copy link
Contributor

@chelseakomlo chelseakomlo commented Nov 29, 2017 •
edited
Loading

This PR does the following:

  • Updates github.com/fsouza/go-dockerclient
  • Allows StopTimeout for docker tasks to be set to kill_timeout

Fixes #3544

schmichael
schmichael requested changes Nov 29, 2017
View reviewed changes
client/driver/docker.go Outdated
User: task.User,
Tty: driverConfig.TTY,
OpenStdin: driverConfig.Interactive,
StopTimeout: int(task.KillTimeout),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be task.KillTimeout.Seconds() as golang Durations are in nanoseconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately task.KillTimeout.Seconds() is of type float64 while StopTimeout is of type int: https://github.com/fsouza/go-dockerclient/pull/693/files#diff-4042076be99ff99c0353b4d447624156R303

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping it in int(...) should convert it just fine (the loss of precision will be meaningless in this case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for the clarification!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here: 5846442

@chelseakomlo chelseakomlo merged commit 619189e into master Dec 4, 2017
@chelseakomlo chelseakomlo deleted the nomad-stop-timeout branch December 4, 2017 15:38
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schmichael schmichael schmichael approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chelseakomlo @schmichael