-
Notifications
You must be signed in to change notification settings - Fork 290
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
Create function to define valid set of scopes for each resource #5558
base: llb-normalized-grants
Are you sure you want to change the base?
Create function to define valid set of scopes for each resource #5558
Conversation
@@ -38,3 +44,20 @@ var Map = map[string]Type{ | |||
Org.String(): Org, | |||
Project.String(): Project, | |||
} | |||
|
|||
// AllowedIn returns the set of Scopes a known Resource type is allowed in. | |||
func AllowedIn(ctx context.Context, r resource.Type) ([]Type, error) { |
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 believe we wanted this to live under the resource package as a resource.Type
method:
const ( |
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.
There are also some other methods in the resource
package that are not methods on resource.Type
. We wanted to update those as well. These methods: Parent
, HasChildTypes
& TopLevelType
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.
Agh, I forgot >.< sorry -- added: a4bafe1
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.
Do you plan on moving AllowedIn to boundary/internal/types/resource/resource.go
? That is where we wanted this to live so resources can just check which scopes they are allowed in with resource.AllowedIn
. Not under the scope
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.
If AllowedIn()
lived in resource
package, we'd have to declare a new scope
domain type in the resource
package. I didn't like the idea of declaring redundant types (ScopeType
and Scope
), hence this function living in the scope
package.
scope
indirectly imports the resource
package as well, so we can't import scope.Type
into the resource
package
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 just remembered something, there's a comment in internal/types/resource/resource.go
that talks about the places that need to be updated when a new resource type is added. Do we want to add something there to mention the new AllowedIn
function?
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 think golangci-lint has a bad cache again, don't worry about those errors.
…ceType` This allows us to call scope.AllowedIn() in one place vs in each implementation of `validScopeTypes`
ef5ae30
to
b46ccd7
Compare
@@ -142,22 +143,22 @@ var Map = map[string]Type{ | |||
|
|||
// Parent returns the parent type for a given type; if there is no parent, it | |||
// returns the incoming type | |||
func Parent(in Type) Type { | |||
switch in { | |||
func (r Type) Parent() Type { |
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.
Will also need to update the test in resource_test.go to update all the function signature changes in here
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.
Updated: 25fd85d
…new resource type
b46ccd7
to
ed329f3
Compare
@@ -38,3 +44,20 @@ var Map = map[string]Type{ | |||
Org.String(): Org, | |||
Project.String(): Project, | |||
} | |||
|
|||
// AllowedIn returns the set of Scopes a known Resource type is allowed in. | |||
func AllowedIn(ctx context.Context, r resource.Type) ([]Type, error) { |
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.
Do you plan on moving AllowedIn to boundary/internal/types/resource/resource.go
? That is where we wanted this to live so resources can just check which scopes they are allowed in with resource.AllowedIn
. Not under the scope
@@ -57,8 +57,8 @@ func (a *authAccount) VetForWrite(ctx context.Context, r db.Reader, opType db.Op | |||
return nil | |||
} | |||
|
|||
func (a *authAccount) validScopeTypes() []scope.Type { | |||
return []scope.Type{scope.Global, scope.Org} | |||
func (a *authAccount) getResourceType() resource.Type { |
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.
Is there a reason we had to update this method?
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.
Technically no -- I previously had validScopeTypes()
return scope.AllowedIn(<some resource type>)
. I'm fine with either change - can revert if that change would be easier to understand
No description provided.