-
Notifications
You must be signed in to change notification settings - Fork 197
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
chore: Refactor registry to be more reusable #1955
chore: Refactor registry to be more reusable #1955
Conversation
cbbf85b
to
949c622
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.
It's weird that this file shows up here. (Since it looks good, no need to clean it up)
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'll rebase once we merge #1956 so this goes away
return registration.model, nil | ||
} | ||
|
||
func PreferredGVK(gk schema.GroupKind) (schema.GroupVersionKind, bool) { |
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 like this name
|
||
// SupportsIAM returns true if this resource supports IAM (not all GCP resources do). | ||
// An error will be returned if IsDirect(groupKind) is not true. | ||
func SupportsIAM(groupKind schema.GroupKind) (bool, 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 don't see where the registry.SupportsIAM
is called. Mind giving me some pointers?
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.
In pkg/cli/cmd/bulkexport/singleresourceiamclient/singleresourceiamclient.go Generally this refactor is working towards #1877
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.
Got it.
/lgtm /hold one question, otherwise LGTM! And thank you for the change! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
949c622
to
5fb1f8a
Compare
5fb1f8a
to
b715194
Compare
/hold cancel |
We need to move things around to avoid import cycles as we add IAM support.
b715194
to
37ab45c
Compare
/lgtm |
00c444b
into
GoogleCloudPlatform:master
We need to move things around to avoid import cycles as we add IAM
support.