pkg/signal: remove DefaultStopSignal const, and un-export container.DefaultStopTimeout#42717
Merged
cpuguy83 merged 2 commits intomoby:masterfrom Aug 24, 2021
Merged
pkg/signal: remove DefaultStopSignal const, and un-export container.DefaultStopTimeout#42717cpuguy83 merged 2 commits intomoby:masterfrom
cpuguy83 merged 2 commits intomoby:masterfrom
Conversation
867a056 to
ec3a6b3
Compare
Member
Author
|
@cpuguy83 @tianon @AkihiroSuda PTAL - let me know what you think (see the PR description - should remove the default from the client?) (related PR moby/sys#77) |
Member
|
I like option b 😅 |
Member
Author
high-five ❤️ 😂 Ok, I can make changes in this PR, and move the default to a private |
It's not used outside of the package itself Signed-off-by: Sebastiaan van Stijn <[email protected]>
This const was previously living in pkg/signal, but with that package being moved to its own module, it didn't make much sense to put docker's defaults in a generic module. The const from the "signal" package is currenlty used *both* by the CLI and the daemon as a default value when creating containers. This put up some questions: a. should the default be non-exported, and private to the container package? After all, it's a _default_ (so should be used if _NOT_ set). b. should the client actually setting a default, or instead just omit the value, unless specified by the user? having the client set a default also means that the daemon cannot change the default value because the client (or older clients) will override it. c. consider defaults from the client and defaults of the daemon to be separate things, and create a default const in the CLI. This patch implements option "a" (option "b" will be done separately, as it involves the CLI code). This still leaves "c" open as an option, if the CLI wants to set its own default. Unfortunately, this change means we'll have to drop the alias for the deprecated pkg/signal.DefaultStopSignal const, but a comment was left instead, which can assist consumers of the const to find why it's no longer there (a search showed the Docker CLI as the only consumer though). Signed-off-by: Sebastiaan van Stijn <[email protected]>
ec3a6b3 to
e53f65a
Compare
thaJeztah
commented
Aug 11, 2021
| // DefaultStopTimeout is the timeout (in seconds) for the shutdown call on a container | ||
| DefaultStopTimeout = 30 | ||
| // defaultStopSignal is the default syscall signal used to stop a container. | ||
| defaultStopSignal = "SIGTERM" |
Member
Author
There was a problem hiding this comment.
Yes, it's the same as the _unix const now, but I decided to define it in both as it's unlikely going to change, and this keeps it together with the corresponding defaultStopTimeout (which does differ between Unix and Windows)
Member
Author
|
@tianon updated; also opened docker/cli#3245 to implement "b" in the cli |
Member
Author
tianon
approved these changes
Aug 18, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This const was previously living in pkg/signal, but with that package being moved to its own module, it didn't make much sense to put docker's defaults in a generic module.
The const from the "signal" package is currenlty used both by the CLI and the daemon as a default value when creating containers. This put up some questions:
This patch implements option "a" (option "b" will be done separately, as it involves the CLI code). This still leaves "c" open as an option, if the CLI wants to set its own default.
Unfortunately, this change means we'll have to drop the alias for the deprecated pkg/signal.DefaultStopSignal const, but a comment was left instead, which can assist consumers of the const to find why it's no longer there (a search showed the Docker CLI as the only consumer though).