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

Rename int64 timestamp related protobuf fields to <fieldname>NS #7069

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

pgporada
Copy link
Member

@pgporada pgporada commented Sep 6, 2023

Rename all of int64 timestamp fields to <fieldname>NS to indicate they are Unix nanosecond timestamps.

Part 1 of 4 related to #7060

…indicate they are Unix nanosecond timestamps
@pgporada pgporada requested a review from a team as a code owner September 6, 2023 18:33
@pgporada
Copy link
Member Author

pgporada commented Sep 6, 2023

The issued field in sa.proto also seems like it should be a google.protobuf.Timestamp.

message AddCertificateRequest {

  // An issued time. When not present the SA defaults to using
  // the current time.
  int64 issued = 4;

@aarongable
Copy link
Contributor

The issued field in sa.proto also seems like it should be a google.protobuf.Timestamp.

Agreed. I wouldn't trust the // Unix timestamp (nanoseconds) comments to be a full accounting of all places where we use an int64 as a timestamp, we've definitely forgotten to include those a time or two. I'd do a survey of all of our .proto files to find any fields we missed.

@beautifulentropy beautifulentropy changed the title Rename int64 timestamp related fields to <fieldname>NS Rename int64 timestamp related protobuf fields to <fieldname>NS Sep 7, 2023
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! I did a quick search through our various *.proto files for int64 fields and turned up a few more candidates.

int64 revokedAt = 4;

int64 revokedDate = 5;

int64 lastExpirationNagSent = 7;

int64 notAfter = 9;

int64 expires = 3;

int64 created = 10;

int64 time = 1; // In nanoseconds

int64 issued = 4;

int64 expires = 2;

@beautifulentropy beautifulentropy requested a review from a team September 7, 2023 16:18
@pgporada pgporada requested a review from a team September 13, 2023 20:42
aarongable
aarongable previously approved these changes Sep 13, 2023
@pgporada pgporada merged commit 034316e into main Sep 15, 2023
12 checks passed
@pgporada pgporada deleted the surfin-on-the-waves-of-time branch September 15, 2023 17:49
pgporada added a commit that referenced this pull request Oct 11, 2023
* Adds new `google.protobuf.Timestamp` fields to each .proto file where
we had been using `int64` fields as a timestamp.
* Updates relevant gRPC messages to populate the new
`google.protobuf.Timestamp` fields in addition to the old `int64`
timestamp fields.
* Added tests for each `<x>ToPB` and `PBto<x>` functions to ensure that
new fields passed into a gRPC message arrive as intended.
* Removed an unused error return from `PBToCert` and `PBToCertStatus`
and cleaned up each call site.

Built on-top of #7069
Part 2 of 4 related to
#7060
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