Skip to content

Add support for details on service logs#42

Merged
cpuguy83 merged 1 commit into
docker:masterfrom
dperny:service-logs-support-details
May 17, 2017
Merged

Add support for details on service logs#42
cpuguy83 merged 1 commit into
docker:masterfrom
dperny:service-logs-support-details

Conversation

@dperny
Copy link
Copy Markdown
Contributor

@dperny dperny commented May 8, 2017

Adds CLI and client support for details on service logs. CLI component of moby/moby#32996.

Comment thread client/parse_logs.go
// string is not in a valid format
// the exact form of details encoding is implemented in
// api/server/httputils/write_log_stream.go
func ParseLogDetails(details string) (map[string]string, error) {
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.

unit test for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do right now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually this is in the wrong place

@dperny dperny force-pushed the service-logs-support-details branch 6 times, most recently from b22ea84 to 7230906 Compare May 12, 2017 00:27
@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 12, 2017

includes #45 because i need tests on docker to pass and they won't without this

@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 16, 2017

please ping me directly on slack when you're reviewing this.

@dperny dperny force-pushed the service-logs-support-details branch 2 times, most recently from 66a2871 to 131f9be Compare May 16, 2017 19:05
@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 16, 2017

rebased onto master. vendored moby at the dependent commit.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

had a quick glance; left two comments

// options specific to service logs
flags.BoolVar(&opts.noResolve, "no-resolve", false, "Do not map IDs to Names in output")
flags.BoolVar(&opts.noTrunc, "no-trunc", false, "Do not truncate output")
flags.BoolVar(&opts.raw, "raw", false, "Do not neatly format logs")
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.

can you add an API version annotation here?

flags.SetAnnotation("raw", "version", []string{"1.30"})

flags.BoolVarP(&opts.follow, "follow", "f", false, "Follow log output")
flags.StringVar(&opts.since, "since", "", "Show logs since timestamp (e.g. 2013-01-02T13:23:37) or relative (e.g. 42m for 42 minutes)")
flags.BoolVarP(&opts.timestamps, "timestamps", "t", false, "Show timestamps")
flags.BoolVar(&opts.details, "details", false, "Show extra details provided to logs")
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.

can you add an API version annotation here?

flags.SetAnnotation("details", "version", []string{"1.30"})

@dperny dperny force-pushed the service-logs-support-details branch from 131f9be to 3043122 Compare May 16, 2017 19:12
@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 16, 2017

@thaJeztah fixed.

Comment thread cli/command/service/logs.go Outdated
} else {
output = append(output, part...)
// then sort em
sort.Strings(detailKeys)
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.

I don't think you need this. I'm pretty sure you can just sort the d slice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, i think you're right. this is hold-over code from when i was caching the details keys.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@dperny dperny force-pushed the service-logs-support-details branch from 3043122 to 93240c2 Compare May 16, 2017 19:28
@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 16, 2017

@dnephin fixed.

@thaJeztah thaJeztah added this to the 17.06.0 milestone May 16, 2017
@aaronlehmann
Copy link
Copy Markdown

Needs a rebase

@dperny dperny force-pushed the service-logs-support-details branch from 93240c2 to 9025f7f Compare May 16, 2017 22:41
@dperny
Copy link
Copy Markdown
Contributor Author

dperny commented May 16, 2017

took the wrong vendor commit on rebase, i force-pushed change but i think gh is broken right now. this should fix it, though.

Adds CLI and client support for details on service logs. CLI component
of moby/moby#32996.

Signed-off-by: Drew Erny <drew.erny@docker.com>
@dperny dperny force-pushed the service-logs-support-details branch from a96dd33 to ebc0eff Compare May 16, 2017 23:54
Copy link
Copy Markdown
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐹

Copy link
Copy Markdown
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
Remove unneeded line in 17.06 CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants