Skip to content

Fix psFormat's Size handling in config file - #3640

Merged
thaJeztah merged 1 commit into
docker:masterfrom
photra:3632-fix-ps-format
Jun 7, 2022
Merged

Fix psFormat's Size handling in config file#3640
thaJeztah merged 1 commit into
docker:masterfrom
photra:3632-fix-ps-format

Conversation

@photra

@photraphotra commented May 31, 2022

Copy link
Copy Markdown
Contributor

- What I did

- How I did it

  1. Added check for Size usage in config file's PsFormat, and moved this logic before calling containerList
  2. Added WithSize function in test builder to allow specifying container size in unit tests
  3. Amended TestContainerListWithConfigFormat to test for changes above

- How to verify it
Run the full test suite via docker buildx bake test, then attempt to reproduce issue above

- Description for the changelog
Fixed incorrect size handling in config file when using docker ps

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

@codecov-commenter

codecov-commenter commented May 31, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3640 (f033b10) into master (84b86e2) will decrease coverage by 0.00%.
The diff coverage is 64.70%.

@@ Coverage Diff @@## master #3640 +/- ##
==========================================
- Coverage 59.01% 59.00% -0.01% 
==========================================
Files 287 287 Lines 24622 24635 +13 ==========================================
+ Hits 14530 14537 +7 - Misses 9218 9222 +4 - Partials 874 876 +2 

@photra

Copy link
Copy Markdown
ContributorAuthor

@thaJeztah thanks for merging my deflake PR the other day! Can you review this one as well?

Comment threadcli/command/container/list.go Outdated
Comment threadinternal/test/builders/container.go
@photra

Copy link
Copy Markdown
ContributorAuthor

@thaJeztah looks like we're all set to merge! Thank you for working on this with me 😄 I learned a lot in the process!

@thaJeztah

Copy link
Copy Markdown
Member

Awesome! Changes look good 😅 - let me do a quick rebase and squash

- do an early check if a custom format is specified either through the
command-line, or through the cli's configuration, before adjusting
the options (to add "size" if needed).
- also removes a redundant `options.Size = opts.size` line, as this value is
already copied at the start of buildContainerListOptions()
- Update NewContainerFormat to use "table" format as a default if no format
was given.
Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Phong Tran <tran.pho@northeastern.edu>
@thaJeztah
thaJeztahforce-pushed the 3632-fix-ps-format branch from d376a19 to 0929bedCompareJune 7, 2022 10:50
@thaJeztahthaJeztah added this to the 22.06.0 milestone Jun 7, 2022

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!!

@thaJeztah

Copy link
Copy Markdown
Member

CI is happy as well; let's bring this one in 👍

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"docker ps --size" should be supported in psFormat config docker ps using psFormat in config that contains Size does not behave as expected

3 participants

@photra@codecov-commenter@thaJeztah