Skip to content
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

Add system time to /info #11291

Merged
merged 1 commit into from Mar 11, 2015
Merged

Add system time to /info #11291

merged 1 commit into from Mar 11, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 10, 2015

This change adds daemon's system time as RFC3339Nano to the /info endpoint
and shows in a more readable format (UnixDate) in docker -D info output.

I will be using this to fix the clock skew between the remote test host and
the CI machines running docker events-related tests as they're using --since
and --until and the timestamps are not matching when daemon is not on the
same machine.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @icecrime @duglin @jfrazelle

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Mar 10, 2015

@jfrazelle we'll need to refresh all the remote test daemon binaries once this is merged to fix the events-related tests on windows CI 😭 😭 just giving a heads up.

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Mar 10, 2015

cool cool

On Tue, Mar 10, 2015 at 2:34 PM, Ahmet Alp Balkan notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle we'll need to refresh all the
remote test daemons once this is merged to fix the events-related tests on
windows CI [image: 😭] [image: 😭] just giving a heads up.


Reply to this email directly or view it on GitHub
#11291 (comment).

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Mar 10, 2015

LGTM

@@ -69,6 +70,15 @@ func (env *Env) SetBool(key string, value bool) {
}
}

func (env *Env) GetDate(key string) (time.Time, error) {
Copy link
Collaborator

@tiborvass tiborvass Mar 10, 2015

Choose a reason for hiding this comment

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

should be GetTime and SetTime

Copy link
Contributor Author

@ahmetb ahmetb Mar 10, 2015

Choose a reason for hiding this comment

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

@tiborvass right. fixed.

This change adds daemon's system time as RFC3339Nano to the `/info` endpoint
and shows in a more readable format (UnixDate) in `docker -D info` output.

I will be using this to fix the clock skew between the remote test host and
the CI machines running `docker events`-related tests as they're using `--since`
and `--until` and the timestamps are not matching when daemon is not on the
same machine.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Mar 11, 2015

Anyone else? 👀

@@ -69,6 +70,15 @@ func (env *Env) SetBool(key string, value bool) {
}
}

func (env *Env) GetTime(key string) (time.Time, error) {
t, err := time.Parse(time.RFC3339Nano, env.Get(key))
return t, err
Copy link
Contributor

@icecrime icecrime Mar 11, 2015

Choose a reason for hiding this comment

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

Not a blocker, but why not directly return time.Parse(...)?

Copy link
Contributor Author

@ahmetb ahmetb Mar 11, 2015

Choose a reason for hiding this comment

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

erm, right. should I fix? @icecrime

Copy link
Contributor

@icecrime icecrime Mar 11, 2015

Choose a reason for hiding this comment

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

We'll survive :p

@icecrime
Copy link
Contributor

@icecrime icecrime commented Mar 11, 2015

Yup, LGTM!

Moving to 3-docs-review: some of the samples might need updating.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Mar 11, 2015

@icecrime updated Remote API 1.18 already.

@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Mar 11, 2015

examples in the Docs LGTM - @docker/docs-owners

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Mar 11, 2015

LGTM

moxiegirl added a commit that referenced this issue Mar 11, 2015
@moxiegirl moxiegirl merged commit d010bf0 into moby:master Mar 11, 2015
1 check passed
@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Mar 11, 2015

Thanks for reviewing. @jfrazelle we need to update remote test daemon bits with this now. 😎

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Mar 11, 2015

woohooo

@ahmetb ahmetb deleted the info/system-date branch Mar 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants