Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Orgid uint#885

Merged
Dieterbe merged 2 commits intomasterfrom
orgid-uint
Apr 13, 2018
Merged

Orgid uint#885
Dieterbe merged 2 commits intomasterfrom
orgid-uint

Conversation

@Dieterbe
Copy link
Copy Markdown
Contributor

@Dieterbe Dieterbe commented Apr 11, 2018

this helps us save some memory.
math.MaxUint32 == 4294967295

I don't anticipate we will ever have more orgs than that.

when loading from cassandra, if <0 -> assign OrgIdPublic

should result in memory savings in the index
when loading for cassandra, if <0 -> assign OrgIdPublic
@Dieterbe Dieterbe requested review from replay and woodsaj April 11, 2018 12:27
}
org, err := strconv.Atoi(orgStr)
if err != nil {
if err != nil || org < 1 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now the public org id is configurable, right? so if somebody sets it to a high value, would 0 as an actual non-public org id be invalid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is for authentication. when authenticating as an org-id, the org id must be >= 1 otherwise it is invalid.
this was true when public org id was -1, and remains true now that public org id can be any value >=1
(you would typically not specify the public org id for authentication, but rather a concrete, real org id)

Copy link
Copy Markdown
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

LGTM with a comment

@Dieterbe Dieterbe merged commit 52d5e51 into master Apr 13, 2018
@Dieterbe Dieterbe deleted the orgid-uint branch April 20, 2018 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants