Promote Github PR ID to int64 #2446
Conversation
Fixes #2443 PR ID and line number are were previously downcasted to int32 in a number of places despite being mostly treated as an int64 type. While the likelihood of either of these values exceeding a 32-bit MAX_INT are minimal, this change makes them 64-bit to err on the side of caution.
dmjb
changed the title
proto/minder/v1/minder.proto
Outdated
| @@ -253,7 +253,7 @@ message PrContents { | |||
| repeated Line patch_lines = 3; | |||
|
|
|||
| message Line { | |||
| int32 line_number = 1; | |||
| int64 line_number = 1; | |||
There was a problem hiding this comment.
I'd leave this as an int32, because I'm terrified of needing more than 2B line numbers for a patch.
There was a problem hiding this comment.
Yeah, if there's a PR file exceeding 2^32 (well, 2147483647 considering the "-" in) number of lines, this counter is the last thing to worry about 😄
| @@ -216,7 +216,7 @@ func ingestFileForFullDiff(filename, patch, patchUrl string) (*pb.PrContents_Fil | |||
| } else if strings.HasPrefix(line, "+") { | |||
| result = append(result, &pb.PrContents_File_Line{ | |||
| Content: line[1:], | |||
| LineNumber: int32(currentLineNumber), | |||
| LineNumber: currentLineNumber, | |||
There was a problem hiding this comment.
We could just declare currentLineNumber as int32 here on line 206, particularly since we parse it as an int32 on line 212.
There was a problem hiding this comment.
Tried this, but since strconv.ParseInt returns (int64, error) we can't just wrap the return value in int32().
| @@ -735,7 +735,7 @@ func getPullRequestInfoFromPayload( | |||
|
|
|||
| return &pb.PullRequest{ | |||
| Url: prUrl, | |||
| Number: int32(prNumber), | |||
| Number: int64(prNumber), | |||
There was a problem hiding this comment.
I'm assuming we can't use JQReadFrom[int64] on line 721 because the underlying API understands that JSON numbers are floats?
There was a problem hiding this comment.
I think so. There is an issue which tracks this: #965
dmjb
changed the title
if diffs are this big, we have problems anyway
dmjb
changed the title
dmjb
changed the title
Summary
Fixes #2443
PR ID and line number are were previously downcasted to int32 in a number of places despite being mostly treated as an int64 type. While the likelihood of either of these values exceeding a 32-bit MAX_INT are minimal, this change makes them 64-bit to err on the side of caution.
EDIT: After discussion with the team, we have decided to leave the line numbers as int32. The rationale is that if the diffs ever get that large, we're going to have a whole bunch of other problems to deal with.
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: