Conversation
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
|
I pushed some more commits to migrate services deps to chrono. Others should follow the same pattern. Must go to sleep now, though >~< |
Signed-off-by: tison <[email protected]>
Thank you for this! |
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
|
All migrated. Then we'd see how this "breaking change" would impact our users. I wonder if we'd wrap jiff's types and methods behind reqsign-core. Or anyway we'd deliver reqsign with jiff since it's the better option now. |
services/azure-storage/src/provide_credential/azure_pipelines.rs
Outdated
Show resolved
Hide resolved
Signed-off-by: tison <[email protected]>
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this! Maybe @BurntSushi wanna take another look?
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Overall LGTM! I left a few suggestions, but I don't see anything that is incorrect. :-)
| /// - Day must be 2 digit. | ||
| pub fn format_http_date(t: DateTime) -> String { | ||
| t.format_with_items(HTTP_DATE.iter()).to_string() | ||
| t.strftime("%a, %d %b %Y %T GMT").to_string() |
There was a problem hiding this comment.
Would it make sense to use an explicit RFC 9110 printer instead?
Note that I can't immediately spot anything obviously wrong with what you have here. Probably the dedicated printer is faster, but I'm not sure if that matters given the String alloc here anyway.
There was a problem hiding this comment.
Good to know. Updating.
There was a problem hiding this comment.
After a closer look, I'd prefer the current consistent usage of strftime as it's correct.
Would you elaborate a bit on how switching to the RFC 9110 printer can benefit?
There was a problem hiding this comment.
Probably the dedicated printer is faster, but I'm not sure if that matters given the String alloc here anyway
Yeah .. I'd prefer code consistency for understandability unless a break has a strong reason.
There was a problem hiding this comment.
Possibly perf. And I think it makes the code clearer to use a dedicated printer. But that's your call.
| // Parse the string format "2023-10-31 21:59:10.000000" | ||
| chrono::NaiveDateTime::parse_from_str(&expires_str, "%Y-%m-%d %H:%M:%S%.f") | ||
| jiff::civil::DateTime::strptime("%Y-%m-%d %H:%M:%S%.f", &expires_str) | ||
| .and_then(|dt| jiff::tz::TimeZone::UTC.to_timestamp(dt)) |
There was a problem hiding this comment.
This makes me somewhat sad. I realize this is what you were doing before, but assuming UTC for datetime string without any time zone or offset is kind of a bummer. It can be rather ambiguous. It's too easy for users to assume the wrong thing (e.g., a local time).
There was a problem hiding this comment.
cc @Xuanwo is this an issue from Azure Storage that they explicitly declare the expires_on string is of UTC but returned without a timezone?
There was a problem hiding this comment.
Confirmed with @Xuanwo that these strings are UTC. It's the vendor's decision so you may report to them directly.
| let nano_time = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| let nano_time = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) |
There was a problem hiding this comment.
I think this is just jiff::Timestamp::now().as_nanosecond(). Although I guess that will behave differently than the code here for clocks set to a time before 1970-01-01T00:00:00Z.
There was a problem hiding this comment.
I get your point. Although, I'd keep the current code as is and leave investigating such a refactor later.
There was a problem hiding this comment.
That said, jiff::Timestamp::now().as_nanosecond() returns i128 where the current nano_time calculation retruns u128. Subtle issues often come from signed/unsigned integers .. Maybe in reality they are the same, but let's investigate it later.
Signed-off-by: tison <[email protected]>
|
Resolved comments updated. |
|
@BurntSushi would you expect a new |
|
Ideally yeah! :-) |
|
@BurntSushi Got it. Typically, an ASF release takes 72 hours for community consensus, but @Xuanwo and I are about to take the National Day holiday, so it may not happen this week. This is on my task list now, anyway :D |
|
Thank you!!! |
3 participants
This closes #631