-
Notifications
You must be signed in to change notification settings - Fork 85
Conversation
Google API best practices say that we should use a string for all publicly visible IDs. If it's not a significant amount of work, we should use a GUID instead of an incrementing int. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I don't think this vector is a concern in this case. If we go with UUIDs, we'd want to do that at the database layer and use that as the primary key instead of having two primary keys (example). |
It's also worth noting that we already use IDs in the URLs for API keys. If we want to switch to UUIDs, we might consider doing that as a wholesale change across all models as a followup? |
pkg/controller/issueapi/issue.go
Outdated
@@ -143,8 +145,13 @@ func (c *Controller) HandleIssue() http.Handler { | |||
} | |||
} | |||
|
|||
var intAsBytes []byte | |||
binary.LittleEndian.PutUint64(intAsBytes, uint64(id)) | |||
idString := base64.StdEncoding.EncodeToString(intAsBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll probably want URLEncoding here instead, since this will be used as part of a URL. However, it's probably better to use hex here (even though it's longer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also document this 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #109
Proposed Changes
Open Question
Release Note