Skip to content

cli/command/container: use ping-result for OS-version#4934

Merged
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:stats_improve
Mar 11, 2024
Merged

cli/command/container: use ping-result for OS-version#4934
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:stats_improve

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Mar 11, 2024

The daemonOSType variable is already set when collecting stats, so we unlikely hit this code in practice, and it would only be set if collect() failed and we never got a stats response. If we do need to get this information, let's use the OSVersion we already obtained from the ping response.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
cli/command/container: use ping-result for OS-version
5c54f75 
The daemonOSType variable is already set when collecting stats, so we unlikely
hit this code in practice, and it would only be set if `collect()` failed and
we never got a stats response. If we do need to get this information, let's use
the OSVersion we already obtained from the ping response.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Mar 11, 2024
@thaJeztah thaJeztah added this to the 26.0.0 milestone Mar 11, 2024
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 11, 2024

Codecov Report

Merging #4934 (5c54f75) into master (952c807) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4934   +/-   ##
=======================================
  Coverage   61.46%   61.47%           
=======================================
  Files         289      289           
  Lines       20229    20226    -3     
=======================================
  Hits        12433    12433           
+ Misses       6895     6892    -3     
  Partials      901      901

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 11, 2024

Flaky test? (TestPromptForConfirmation)

Run go test -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')
  go test -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')
  go tool cover -func=/tmp/coverage.txt
  shell: /bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    GOPATH: /Users/runner/work/cli/cli
    GOBIN: /Users/runner/work/cli/cli/bin
    GO111MODULE: auto
ok  	github.com/docker/cli/cli	0.018s	coverage: 28.2% of statements
panic: send on closed channel

goroutine 104 [running]:
github.com/docker/cli/cli/command_test.TestPromptForConfirmation.func8.2()
	/Users/runner/work/cli/cli/src/github.com/docker/cli/cli/command/utils_test.go:209 +0x6d
created by github.com/docker/cli/cli/command_test.TestPromptForConfirmation.func8 in goroutine 103
	/Users/runner/work/cli/cli/src/github.com/docker/cli/cli/command/utils_test.go:207 +0x3aa
FAIL	github.com/docker/cli/cli/command	0.423s

@thaJeztah thaJeztah self-assigned this Mar 11, 2024
krissetto
krissetto approved these changes Mar 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@krissetto krissetto left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit d06f137 into docker:master Mar 11, 2024
@thaJeztah thaJeztah deleted the stats_improve branch March 11, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@krissetto krissetto krissetto approved these changes

Assignees

@thaJeztah thaJeztah

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Milestone

26.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @codecov-commenter @krissetto