-
Notifications
You must be signed in to change notification settings - Fork 285
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
Add ability to set approvers on views in sdf #5279
Conversation
Dependency Review✅ No vulnerabilities or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
6a4529d
to
6e97c1a
Compare
a268533
to
6ae6549
Compare
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.
👏
6ae6549
to
d5d4620
Compare
This commit adds the ability to set approvers on the views with a new set of sdf routes, backed by a new integration test among existing ones. To support this change, we now divide ApprovalRequirements into two separate categories: explicit and virtual. - Explicit requirements are derived from ApprovalRequirementDefinition nodes (new to WorkspaceSnapshotGraphV4!) on the graph from any node with an outgoing HasApprovalRequirement edge - Virtual requirements are derived from examining a list of changes on the fly and are useful for determining if meta changes to permissions require approvals themselves (e.g. if you add a ApprovalRequirementDefinition, we need to determine who can approve that change) Both types of requirements share an inner "rule" type with covers the universal elements of all approval requirements: - The ID of the entity requiring an approval - The kind of entity requiring an approval - The minimum number of approvals for this rule - The approvers of this rule, which can either be individual users or a "permission lookup" bag used to query SpiceDB for who can approve the requirement Back to virtual requirements: not only are they useful for determining meta changes to permissions, but also for creating requirements when nodes are deleted and creating the checksum for new approvals involving deleted nodes. In fact, that is why checksum calculatation now requires the hashes alongside the IDs... you can't get the merkle tree hash for a deleted node! As a result of these changes, detecting changes between two graphs is now fallible (we need to get the node weight in order to get the merkle tree hash). Other changes in this commit include, but are not limited to, the following: - Use UserPk instead of String for return type involving frontend approvals - Re-order enum variants and their discriminants where possible when dealing with ApprovalRequirementDefinition and HasApprovalRequirement - Rename "geometry" dummy data to "view" in deserialize graph integration test because it could be confusing (although it is entirely cosmetic for the purposes of the test) Co-authored-by: Jacob Helwig <[email protected]> Signed-off-by: Nick Gerace <[email protected]>
d5d4620
to
414540c
Compare
Reading through after check-in: one thing I will be looking for is whether any work needs to be done to support the automation API's force_apply and request_approval endpoints. |
This commit renames the HasApprovalRequirement EdgeWeight to ApprovalRequirementDefinition for clarity. This rename removes hints of directionality and specifically refers to the ApprovalRequirementDefinition node weight. This commit also fixes two debug output locations for the new node weight as well as changes the order (with a comment) for the approver enum variants. Co-authored-by: Jacob Helwig <[email protected]> Signed-off-by: Nick Gerace <[email protected]>
2b658f1
to
749508e
Compare
); | ||
)?; | ||
|
||
// Add changes for IDs that only exist in the base graph because our DFS walk of the |
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.
can you elaborate on this a bit for me?
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.
Because we're walking the change set's graph, we won't encounter things that have been deleted, such as an approval requirement definition. Because we want all approval requirement definition changes (add/remove/modify) to require approval from the workspace approvers we need to know which things only exist in the base graph (and what kind of thing they are) to be able to later generate a "virtual" approval requirement where appropriate for them.
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.
ahhh makes total sense!
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'm not following the virtual vs. explicit here - let's chat tomorrow so I can understand
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.
"Virtual" are approval requirements that do not come from an ApprovalRequirementDefinition
node in the graph (an "explicit" definition that generates an explicit requirement). The biggest example right now would be for ApprovalRequirementDefinition
nodes themselves. If we made an ApprovalRequirementDefinition
for the ApprovalRequirementDefinition
nodes then we'd have to also define what the approval requirements are for add/remove/modify of that, and it would end up as ApprovalRequirementDefinition
chains all the way down. To avoid the infinite chain of ApprovalRequirementDefinition
we say that a change involving an ApprovalRequirementDefinition
will inherently generate a "virtual" requirement for itself, and that it's "virtual" lets us know that there is not an ApprovalRequirementDefinition
we can go back to and add/modify/remove to change the required number of approvers, which groups can approve, or which individuals can approve, as it's a broader systemic rule that caused it to exist.
Eventually, as we continue adding in more and more of the fallback logic and towards migrating the graph so that there are ApprovalRequirementDefinition
nodes to represent some of the current in-code defaults, the only thing that would be using/generating the "virtual" requirements would be ApprovalRequirementDefinition
. Projecting out even a little further, we may end up being able to remove it entirely if we add some workspace-level configuration to the graph (probably one or more nodes off the root) that would indicate "This is an ApprovalRequirementDefinition
for all nodes of kind X". If we had that, then we'd both have a concrete definition to point back to as the reason for why the requirement was generated (and therefore what you would need to modify if you wanted to change what requirements are generated), and it would allow people to configure the required number of approvers, which group(s), which people, etc., instead of it being globally hard-coded for everyone.
If we didn't care about being able to say "this approval requirement exists because of that specific definition over there" then we wouldn't need the virtual/explicit breakdown with the current setup, but I think it'll be very important that we're capable of providing that kind of information to users until all of the requirements eventually come from something in the graph itself and we no longer need to say "this approval requirement exists because of a definition you cannot modify".
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.
this is exactly what I was looking for - super super helpful, thank you!!
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 pulled this branch and tested locally the existing approval flow. Approvers can still force apply, and contributors still get bumped to the existing approval flow. Tested both with creating new components/updating existing and creating schema variants.
Thank you very much for the review @britmyerss!! |
Introduction
This PR adds the ability to set approvers on the views with a new set of
sdf
routes, backed by a new integration test among existing ones.Description
To support this change, we now divide
ApprovalRequirements
into two separate categories: explicit and virtual.ApprovalRequirementDefinition
nodes (new toWorkspaceSnapshotGraphV4
!) on the graph from any node with an outgoingHasApprovalRequirement
edgeApprovalRequirementDefinition
, we need to determine who can approve that change)Both types of requirements share an inner "rule" type with covers the universal elements of all approval requirements:
Back to virtual requirements: not only are they useful for determining meta changes to permissions, but also for creating requirements when nodes are deleted and creating the checksum for new approvals involving deleted nodes. In fact, that is why checksum calculation now requires the hashes alongside the IDs... you can't get the merkle tree hash for a deleted node!
As a result of these changes, detecting changes between two graphs is now fallible (we need to get the node weight in order to get the merkle tree hash).
Other Changes
Other changes in this PR include, but are not limited to, the following:
UserPk
instead ofString
for return type involving frontend approvalsApprovalRequirementDefinition
andHasApprovalRequirement
Relevant PRs
This is a follow-up to #5144