Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Support format option in docker app ls #743

Merged
merged 6 commits into from
Nov 20, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 13, 2019

- What I did
Introduced --format experimental option for docker app ls
Can either pass a Go template (jist like most docker CLI commands) or just "json" to enable json output.

- How I did it
with love

- How to verify it
docker app ls --format "{{(index . 0).Reference}}"
docker app ls --format "json"

- Description for the changelog
Introduced --format experimental option for docker app ls

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

Using either plain JSON or a go template

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #743 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #743   +/-   ##
=======================================
  Coverage   70.81%   70.81%           
=======================================
  Files          64       64           
  Lines        3663     3663           
=======================================
  Hits         2594     2594           
  Misses        748      748           
  Partials      321      321

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 355ab4f...d5e1ff5. Read the comment docs.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@rumpl
Copy link
Member

rumpl commented Nov 14, 2019

Could we add a test for this?

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof force-pushed the template branch 4 times, most recently from 9da966a to f723638 Compare November 20, 2019 07:53
default:
return defaultImageTableFormat
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I should have written it like:

if source == formatter.TableFormatKey {
        if quiet {
                return formatter.DefaultQuietFormat
        }
        if digest {
                return defaultImageTableFormatWithDigest
        }
        return defaultImageTableFormat
}

I don't see the usage of switch with only one case and switch on different variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this code was a (simplified) version of the original formatter from docker/cli. Wanted to keep implementations in sync

Copy link
Member

Choose a reason for hiding this comment

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

OK, I understand.

type imageContext struct {
formatter.HeaderContext
trunc bool
i imageDesc
Copy link
Member

Choose a reason for hiding this comment

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

Why not ?

Suggested change
i imageDesc
image imageDesc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason

@ndeloof ndeloof force-pushed the template branch 2 times, most recently from d30c6d7 to cc7360f Compare November 20, 2019 12:25
set by
cli/command/formatter/formatter.go#postFromat

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@silvin-lubecki silvin-lubecki merged commit df7ae09 into docker:master Nov 20, 2019
@ndeloof ndeloof deleted the template branch November 20, 2019 14:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants