fix: use timezone-aware datetimes in expiry calculation and file info#2378
fix: use timezone-aware datetimes in expiry calculation and file info#2378tonyandrewmeyer wants to merge 3 commits intocanonical:mainfrom
Conversation
tonyandrewmeyer
force-pushed
the
fix/timezone-aware-datetimes
branch
2 times, most recently
from
9be875b to
bf84978
Compare
Use `datetime.datetime.now(tz=datetime.timezone.utc)` in `_calculate_expiry()` and `datetime.datetime.fromtimestamp(..., tz=datetime.timezone.utc)` in `Container._build_fileinfo()` so that the resulting datetimes carry UTC timezone information. Naive datetimes are problematic because `datetime.isoformat()` omits the timezone suffix, producing strings that do not conform to RFC 3339 as expected by Juju. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tonyandrewmeyer
force-pushed
the
fix/timezone-aware-datetimes
branch
from
bf84978 to
7a0e393
Compare
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
The _calculate_expiry change seems correct.
Does this fix an active bug or one that we'd hit with a future Juju release? The PR description isn't clear, e.g.
Juju expects RFC 3339 timestamps with timezone information.
Implies that our timezone-naive timezones might be being rejected or treated oddly by Juju (perhaps it treats timestamps without timezone info as UTC, which will be incorrect in most timezones?).
The _build_fileinfo change appears to be a no-op (see comment), but is more correct, so that one doesn't need discussion.
test/test_model.py
Outdated
|
|
||
| @mock.patch('pwd.getpwuid') | ||
| @mock.patch('grp.getgrgid') | ||
| def test_build_fileinfo_returns_utc_timestamp( | ||
| self, getgrgid: mock.MagicMock, getpwuid: mock.MagicMock | ||
| ): | ||
| """Container._build_fileinfo should return a timezone-aware last_modified.""" | ||
| getpwuid.side_effect = KeyError | ||
| getgrgid.side_effect = KeyError | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| path = pathlib.Path(tmpdir) / 'test.txt' | ||
| path.write_text('hello') | ||
| fileinfo = ops.Container._build_fileinfo(path) | ||
|
|
||
| assert fileinfo.last_modified.tzinfo is not None, ( | ||
| '_build_fileinfo should return a timezone-aware last_modified' | ||
| ) | ||
| assert fileinfo.last_modified.tzinfo == datetime.timezone.utc |
There was a problem hiding this comment.
We probably don't really need a test for this, since it's internal only and doesn't affect anything as far as I can tell.
| def test_calculate_expiry_with_datetime_returns_as_is(self): | ||
| """_calculate_expiry with a datetime should return it unchanged.""" | ||
| dt = datetime.datetime(2025, 6, 1, 12, 0, 0, tzinfo=datetime.timezone.utc) | ||
| result = _calculate_expiry(dt) | ||
| assert result is dt |
There was a problem hiding this comment.
Perhaps more useful to highlight in this test that a timezone-naive datetime is returned unchanged.
dimaqq
left a comment
dimaqq
left a comment
There was a problem hiding this comment.
The change looks solid.
I'm curious how come this was not an issue until now.
Also, please validate that some old Juju, like 2.9.x honours the new timestamps. I'd assume that Juju did from the get-go, being coded in Go, but better safe than sorry.
4 participants
Summary
_calculate_expiry()(line 539): Changeddatetime.datetime.now()todatetime.datetime.now(tz=datetime.timezone.utc)so that when a charm passes atimedeltafor secret expiry, the resulting datetime is timezone-aware. Previously, the naive datetime would be serialised viadatetime_to_rfc3339(which calls.isoformat()), producing a string without a timezone suffix. Juju expects RFC 3339 timestamps with timezone information.Container._build_fileinfo()(line 3047): Changeddatetime.datetime.fromtimestamp(info.st_mtime)todatetime.datetime.fromtimestamp(info.st_mtime, tz=datetime.timezone.utc)so thatFileInfo.last_modifiedcarries UTC timezone information rather than being a naive datetime in the local timezone.Both fixes ensure that datetimes produced by these functions include explicit UTC timezone data, which is necessary for correct RFC 3339 serialisation and consistent behaviour across different host timezone configurations.
🤖 Generated with Claude Code but owned by me.