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: handle deprecation of datetime.utcnow() #1908

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 9, 2025

I saw this warning in the logging of another PR.

I have not been able to test this locally (or rather, I didn't want to spin up a minikube cluster).

Hopefully the CI catches any regressions.

capsule = {
"timestamp": datetime.utcnow().isoformat() + "Z",
"timestamp": now_utc.isoformat() + "Z",
Copy link
Member

Choose a reason for hiding this comment

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

the +Z here works for adding the UTC timezone indicator to a naïve datetime, but now_utc is now a tz-aware datetime, so this produces:

2025-01-23T08:33:01.361903+00:00Z

We can do one of:

  1. use isoformat as-is with the new tz-aware datetime and stop adding the Z (+00:00 is equally valid, if not as common for always-utc machine timestamps)
  2. use `.isoformat().replace("+00:00", "Z")
  3. keep now_utc as naïve by adding .replace(tzinfo=None) to its construction
  4. use strftime('%Y-%m-%dT%H:%M:%SZ') to explicitly format with Z instead of utc offset

I don't really have a preference, so whichever strikes your fancy. I guess the simplest and most consistent with before is option 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@minrk huh, good spot! I didn't see that utc_now was already tz aware.

I like your presentation of the options. My preference is for (4), as it explicitly defines the spec that we're formatting for the recipients, and doesn't break anyone already relying on the format of the timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think option 4 is better to backward compatibility. I'm also happy with option 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change, and restored microseconds.

@minrk minrk merged commit 243fd34 into jupyterhub:main Jan 23, 2025
14 checks passed
@minrk
Copy link
Member

minrk commented Jan 23, 2025

Thanks!

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 23, 2025
jupyterhub/binderhub#1908 Merge pull request #1908 from agoose77/agoose77/fix-deprecation-utcnow
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.

3 participants