This repository was archived by the owner on Aug 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 107
public orgId -1 -> 0 and related cleanups #880
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
7b798d3
remove special case Prune(-1); we always want all-orgs
Dieterbe 56e44d0
remove special case idx.List(-1)
Dieterbe dee84b1
make "public org" explicit + optimize find in case orgIdPublic passed
Dieterbe 5db1111
migrate orgId for public data to 0
Dieterbe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
shouldn't we just make this configurable with a command line arg?
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.
what's the benefit of that?
I think this should be a constant across the entire stack and the surrounding ecosystem of tools:
org 0 = public data
org 1 = admin org
org >1 = private tenant orgs
making it configurable seems to cause nothing but complications.
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.
Why does the public org need to be 0. Wasnt the whole point of using a variable (or const) to remove the need to have
special
values.For the Wolrdping case, we currently have to hard code the collection of the public data into the probe. But we could simplify everything by just creating a
public
org and configure the endpoints for it, known that the data collected for that org will be viewable by all users.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.
No, the point was to only use orgid's that are >=0 so that they are encodeable as a uint. this is also what was explained in the original ticket #260
there's nothing wrong with special reserved values per se. I think conventions are useful, as long as they are simple and consistently applied. they simplify things (less config flags to worry about and keep in sync across different software, clearer code)
Your proposed simplification for WP probes sounds nice, but does it really conflict with the proposed convention? is it not possible to just create that public org with id 0 ?
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.
I dont really like using the zero value, as it then makes it impossible to know if the value was set or not.
But as we dont have a use case for needing non-zero right now, lets just leave this. It will be trivial to change this later if we want/need to.