-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: direct controller for SecureSourceManagerInstance #2478
base: master
Are you sure you want to change the base?
feat: direct controller for SecureSourceManagerInstance #2478
Conversation
justinsb
commented
Aug 12, 2024
- feat: direct controller for SecureSourceManagerInstance
- tests: add basic test for SecureSourceManagerInstance
588a7b6
to
6d838c6
Compare
"sigs.k8s.io/controller-runtime/pkg/client" | ||
) | ||
|
||
func normalizeResourceName(ctx context.Context, reader client.Reader, src client.Object, ref *v1alpha1.ResourceRef) (*v1alpha1.ResourceRef, 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.
This is moved from monitoring. My plan is to put it into a shared place, then remove duplicates / refactor etc. Open to alternative suggestions!
"reflect" | ||
) | ||
|
||
type Visitor interface { |
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.
Also moved from monitoring
1c163ea
to
41e5f28
Compare
301efe3
to
2cf1758
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.
/lgtm
/approve
/hold
The controller is a little bit out of date. No blockers, feel free to unhold.
"k8s.io/apimachinery/pkg/runtime" | ||
) | ||
|
||
func SetObservedState(u *unstructured.Unstructured, typedObservedState any) 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.
Can you add a comment to introduce how we shall (and shall not) use this function?
type instanceAdapter struct { | ||
projectID string | ||
location string | ||
resourceID string |
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.
Mind adding a todo about using externalRef instead of the resourceID? Just in case we forget it.
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.
oh, because https://github.com/GoogleCloudPlatform/k8s-config-connector/pull/2370/files is merged after you run the controller template. (was wondering why this controller contains many out of date changes).
[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 |