Skip to content

Do not set a default timeout of 10 seconds when restarting / stopping…#10672

Merged
milas merged 1 commit intodocker:v2from
easyflex:10670-no_default_timeout
Jun 12, 2023
Merged

Do not set a default timeout of 10 seconds when restarting / stopping…#10672
milas merged 1 commit intodocker:v2from
easyflex:10670-no_default_timeout

Conversation

@robbert-ef
Copy link
Copy Markdown
Contributor

@robbert-ef robbert-ef commented Jun 8, 2023

… but use the container StopTimeout (defaults to 10 seconds)

Also fixed custom timeout not used when invoking up

What I did
The default timeout value of 10 seconds when using stop / restart was removed.
This now defaults to the container StopTimeout which can be set using stop_grace_period and defaults to 10 seconds.
Also fixed custom timeout value not being used when calling docker compose up

Related issue
fixes #10670

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@ndeloof
Copy link
Copy Markdown
Contributor

ndeloof commented Jun 8, 2023

for legal reasons we require commits to be signed-off. Please amend your commit and force push your branch

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 8, 2023

Codecov Report

Patch coverage: 71.42% and project coverage change: -0.05 ⚠️

Comparison is base (599723f) 58.91% compared to head (d035d2b) 58.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10672      +/-   ##
==========================================
- Coverage   58.91%   58.87%   -0.05%     
==========================================
  Files         111      111              
  Lines        9709     9716       +7     
==========================================
  Hits         5720     5720              
- Misses       3401     3407       +6     
- Partials      588      589       +1     
Impacted Files Coverage Δ
cmd/compose/restart.go 68.42% <60.00%> (-5.78%) ⬇️
cmd/compose/down.go 79.16% <100.00%> (ø)
cmd/compose/stop.go 78.12% <100.00%> (ø)
cmd/compose/up.go 70.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@robbert-ef robbert-ef force-pushed the 10670-no_default_timeout branch from 41b347d to b384063 Compare June 8, 2023 14:16
@robbert-ef
Copy link
Copy Markdown
Contributor Author

for legal reasons we require commits to be signed-off. Please amend your commit and force push your branch

done

@robbert-ef robbert-ef force-pushed the 10670-no_default_timeout branch from b384063 to ce63150 Compare June 9, 2023 07:45
… but use the container StopTimeout (defaults to 10 seconds)

Also fixed custom timeout not used when invoking `up`

Signed-off-by: Robbert Segeren <[email protected]>
@robbert-ef robbert-ef force-pushed the 10670-no_default_timeout branch from ce63150 to d035d2b Compare June 12, 2023 10:19
@ndeloof ndeloof requested review from a team, StefanScherer, glours, laurazard, milas, nicksieger and ulyssessouza and removed request for a team June 12, 2023 12:52
Copy link
Copy Markdown
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, nice improvement! ✨

Comment thread cmd/compose/up.go
Short: "Create and start containers",
PreRunE: AdaptCmd(func(ctx context.Context, cmd *cobra.Command, args []string) error {
create.pullChanged = cmd.Flags().Changed("pull")
create.timeChanged = cmd.Flags().Changed("waitTimeout")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sometime goland "rename" feature tries to be too clever...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] docker compose restart ignores stop_grace_period / always uses default timeout of 10s

4 participants