-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce GuardedPrebuild to be used in #10696 #10940
Conversation
86bedfd
to
3eb0c84
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.
Looks good and makes sense to me, many thanks!
I guess my biggest question is -- why do we need this extra resource guard? Isn't it enough to guard on access to the workspace
(or on the workspaceLog
)? Do we expect access models for workspaces, logs, and prebuilds to differ significantly in the future?
/hold
workspaceType: "prebuild", | ||
isOwner: false, | ||
teamRole: undefined, | ||
expectation: false, |
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.
@geropl, out of curiosity, what the expected effect of this change for a non-team-member to open a workspace on a configured repository? e.g. is this rule also enforced on workspace startup?
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 PR does just introduce the GuardedPrebuild, but it is not used yet.
A follow up PR will introduce prebuild APIs (get/findByWorkspaceId), and it felt wrong to re-/ab-use workspace or workspaceinstance for it.
3eb0c84
to
0a37cf9
Compare
@jankeromnes Not really, but I'd like to have it explicit. That way it's easier to either a) migrate to other implementations, or b) remove in the future. 🧘 |
/unhold |
Description
Introduces
GuardedPrebuild
to guard operations on Prebuilds.This will be used in a follow-up PR (#10696).
Related Issue(s)
Based on: #10939
Context: #10696
How to test
(components/server && yarn test)
Release Notes
Documentation
Werft options: