New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

30431 implement format for history with docs #30962

Merged
merged 1 commit into from Apr 16, 2017

Conversation

@TheHipbot
Contributor

TheHipbot commented Feb 13, 2017

- What I did
Adds the --format option to the history command. Part of #30431

- How I did it
Refactored history to use a formatter, add the format flag

- How to verify it
Wrote unit tests to check that formatter is working correctly.

- Description for the changelog

Adds --format option to history command

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

@vdemeester

Design LGTM 🐮

@TheHipbot

This comment has been minimized.

Show comment
Hide comment
@TheHipbot

TheHipbot Feb 13, 2017

Contributor

Sorry, I was having trouble running the integration tests, I'll fix this up tonight and update the PR.

Contributor

TheHipbot commented Feb 13, 2017

Sorry, I was having trouble running the integration tests, I'll fix this up tonight and update the PR.

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Feb 14, 2017

@TheHipbot

This comment has been minimized.

Show comment
Hide comment
@TheHipbot

TheHipbot Feb 15, 2017

Contributor

Is this still failing CI?

Contributor

TheHipbot commented Feb 15, 2017

Is this still failing CI?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Feb 20, 2017

Member

I confirmed it works, so code LGTM

  • Please squash commits
  • Probably we don't need an integration test
Member

AkihiroSuda commented Feb 20, 2017

I confirmed it works, so code LGTM

  • Please squash commits
  • Probably we don't need an integration test
@TheHipbot

This comment has been minimized.

Show comment
Hide comment
@TheHipbot

TheHipbot Feb 21, 2017

Contributor

Squashed and removed the integration test.

Contributor

TheHipbot commented Feb 21, 2017

Squashed and removed the integration test.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda
Member

AkihiroSuda commented Feb 21, 2017

@yongtang PTAL?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 21, 2017

Member

@TheHipbot it looks like --human is only useful when --format "table" (or no --format is used):

ubuntu@ubuntu:~/docker$ docker history --format '{{.Created}}' --human=false busybox
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table {{.Created}}' --human=false busybox
CREATED AT
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table {{.Created}}' --human busybox
CREATED AT
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table' --human busybox
IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
7968321274dc        5 weeks ago         /bin/sh -c #(nop)  CMD ["sh"]                   0B                  
<missing>           5 weeks ago         /bin/sh -c #(nop) ADD file:707e63805c0be1a...   1.11MB              
ubuntu@ubuntu:~/docker$ 

You may need to check the value of c.human inside Created() (and/or CreatedSince()) to render the output string in either human or non-human format accordingly. For example,

func (c *historyContext) Created() string {
	c.AddHeader(createdAtHeader)
        if c.human {
          .... 
        }
	return time.Unix(c.h.Created, 0).Format(time.RFC3339)
}

Also, I am wondering if it make sense to merge Created() and CreatedSince()?

Member

yongtang commented Feb 21, 2017

@TheHipbot it looks like --human is only useful when --format "table" (or no --format is used):

ubuntu@ubuntu:~/docker$ docker history --format '{{.Created}}' --human=false busybox
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table {{.Created}}' --human=false busybox
CREATED AT
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table {{.Created}}' --human busybox
CREATED AT
2017-01-13T14:13:54-08:00
2017-01-13T14:13:53-08:00
ubuntu@ubuntu:~/docker$ docker history --format 'table' --human busybox
IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
7968321274dc        5 weeks ago         /bin/sh -c #(nop)  CMD ["sh"]                   0B                  
<missing>           5 weeks ago         /bin/sh -c #(nop) ADD file:707e63805c0be1a...   1.11MB              
ubuntu@ubuntu:~/docker$ 

You may need to check the value of c.human inside Created() (and/or CreatedSince()) to render the output string in either human or non-human format accordingly. For example,

func (c *historyContext) Created() string {
	c.AddHeader(createdAtHeader)
        if c.human {
          .... 
        }
	return time.Unix(c.h.Created, 0).Format(time.RFC3339)
}

Also, I am wondering if it make sense to merge Created() and CreatedSince()?

@TheHipbot

This comment has been minimized.

Show comment
Hide comment
@TheHipbot

TheHipbot Feb 22, 2017

Contributor

@yongtang I was originally thinking to provide both options, which I should have added to the docs. That being said it also makes sense to combine so I will update the PR with the two combined as Created() and update the docs to reflect that

Contributor

TheHipbot commented Feb 22, 2017

@yongtang I was originally thinking to provide both options, which I should have added to the docs. That being said it also makes sense to combine so I will update the PR with the two combined as Created() and update the docs to reflect that

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 6, 2017

Member

Thanks @TheHipbot for the PR update. Can you squash commit into one, and make sure commit is signed (to fix doc-signed error)?

Member

yongtang commented Mar 6, 2017

Thanks @TheHipbot for the PR update. Can you squash commit into one, and make sure commit is signed (to fix doc-signed error)?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi
Member

tonistiigi commented Mar 9, 2017

@yongtang PTAL

@tonistiigi

LGTM

| `.CreatedAt` | Timestamp of when image was created |
| `.CreatedBy` | Command that was used to create the image |
| `.Size` | Image disk size |
| `.Comment` | Comment for image |

This comment has been minimized.

@yongtang

yongtang Mar 9, 2017

Member

Can you update the doc to align the table like below so that it is easy to update in the future?

| Placeholder     | Description |
| --------------- | ----------- |
| `.ID`           | Image ID    |
| `.CreatedSince` | Elapsed time since the image was created if --human=true, otherwise timestamp of when image was created |
| `.CreatedAt`    | Timestamp of when image was created |
| `.CreatedBy`    | Command that was used to create the image |
| `.Size`         | Image disk size |
| `.Comment`      | Comment for image |

@yongtang

yongtang Mar 9, 2017

Member

Can you update the doc to align the table like below so that it is easy to update in the future?

| Placeholder     | Description |
| --------------- | ----------- |
| `.ID`           | Image ID    |
| `.CreatedSince` | Elapsed time since the image was created if --human=true, otherwise timestamp of when image was created |
| `.CreatedAt`    | Timestamp of when image was created |
| `.CreatedBy`    | Command that was used to create the image |
| `.Size`         | Image disk size |
| `.Comment`      | Comment for image |

var created string
created = units.HumanDuration(time.Now().UTC().Sub(time.Unix(int64(c.h.Created), 0)))
return created + " ago"
}

This comment has been minimized.

@yongtang

yongtang Mar 9, 2017

Member

Also, it seems that CreatedAt and CreatedSince is pretty much the same except for the ago. And --human flag does not have an impact to the rendering

I am wondering if CreatedAt should use time.Unix(int64(c.h.Created), 0).String() (just like docker images --format) instead?

@yongtang

yongtang Mar 9, 2017

Member

Also, it seems that CreatedAt and CreatedSince is pretty much the same except for the ago. And --human flag does not have an impact to the rendering

I am wondering if CreatedAt should use time.Unix(int64(c.h.Created), 0).String() (just like docker images --format) instead?

This comment has been minimized.

@TheHipbot

TheHipbot Mar 13, 2017

Contributor

I ended up having to separate these again due to some changes in how the headers are output with the formatters. I added to the docs to have both in the template.

@TheHipbot

TheHipbot Mar 13, 2017

Contributor

I ended up having to separate these again due to some changes in how the headers are output with the formatters. I added to the docs to have both in the template.

Implements --format option for docker history command by creating a f…
…ormatter

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Adds to history documentation for --format

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Adds MarshalJSON to historyContext for {{json .}} format

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Adds back the --human option to history command

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Cleans up formatter around --human option for history, Adds integration test for --format option of history

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Adds test for history formatter checking full table results, Runs go fmt on touched files

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Fixes lint errors in formatter/history

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

Runs go fmt on cli/command/formatter/history.go

Signed-off-by: Jeremy Chambers <jeremy@thehipbot.com>

sRemoves integration test for --format option of history

Merges Created and CreatedSince in docker history formatter, Updates docs and tests
@TheHipbot

This comment has been minimized.

Show comment
Hide comment
@TheHipbot

TheHipbot Mar 13, 2017

Contributor

Looks like Jenkins ran out of space on one of those builds

Contributor

TheHipbot commented Mar 13, 2017

Looks like Jenkins ran out of space on one of those builds

@vdemeester

LGTM 🐮
/cc @thaJeztah for docs review

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi
Member

tonistiigi commented Apr 13, 2017

ping @thaJeztah

@thaJeztah

LGTM

I agree with #30962 (comment), but it's just a nit so let's get this merged. I can do a quick follow up to address it

@thaJeztah thaJeztah merged commit a3ab463 into moby:master Apr 16, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31591 has succeeded
Details
janky Jenkins build Docker-PRs 40213 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 504 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11292 has succeeded
Details
z Jenkins build Docker-PRs-s390x 173 has succeeded
Details

@thaJeztah thaJeztah added this to the 17.06 milestone Apr 16, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30962 from TheHipbot/30431-implement-format-f…
…or-history-with-docs

30431 implement format for history with docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment