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

Fix time argument formatting to include time zone #574

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

kamalmarhubi
Copy link
Contributor

In local testing, I saw that Container.logs misbehaved when passing the since parameter. I believe this is because my time zone is UTC-4, and so asking for something like

container.logs(since=container.state.started_at)

would return no logs as it was effectively asking for logs from the future:

  • container.state.started_at parses the inspect output into a zone-aware datetime; the timezone in inspect's output is always UTC (Z)
  • the time formatting in format_time_for_docker strips the time zone rather than converts to local time
  • the docker CLI treats its --since argument as being in local time

I instrumented the Popen call to see what arguments were passed to the docker CLI. Taking the exact same command and appending a Z to the time to force the correct time zone printed logs as expected.

In local testing, I saw that `Container.logs` misbehaved when passing
the `since` parameter. I believe this is because my time zone is UTC-4,
and so asking for something like

    container.logs(since=container.state.started_at)

would return no logs as it was effectively asking for logs from the
future:
- container.state.started_at parses the inspect output into a zone-aware
  datetime; the timezone in inspect's output is always UTC (`Z`)
- the time formatting in `format_time_for_docker` strips the time zone
  rather than converts to local time
- the docker CLI treats its `--since` argument as being in local time

I instrumented the Popen call to see what arguments were passed to the
docker CLI. Taking the exact same command and appending a `Z` to the
time to force the correct time zone printed logs as expected.
@gabrieldemarmiesse
Copy link
Owner

Many thanks for the thorough bug hunting! That's awesome! From what you described, it sounds like a good fix. I'll take a look in details and give you a review in the next few days :) I also wonder if this kind of bug can occur somewhere else in the codebase.

Copy link
Owner

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

Thanks! Let's merge it in master. Can you try it out with pip install git+https://github.com/gabrieldemarmiesse/python-on-whales.git and tell me if it works?

@gabrieldemarmiesse gabrieldemarmiesse merged commit ac0e53b into gabrieldemarmiesse:master Mar 29, 2024
35 checks passed
@kamalmarhubi
Copy link
Contributor Author

@gabrieldemarmiesse yup I removed my monkey patch to test with current master and it works, thanks!

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.

2 participants