-
Notifications
You must be signed in to change notification settings - Fork 235
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: Promote FirestoreDatabase to v1beta1 #3341
base: master
Are you sure you want to change the base?
feat: Promote FirestoreDatabase to v1beta1 #3341
Conversation
a6ecbcf
to
c47dba4
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.
Suggest dropping the change under pkg/clients/generated
because of this https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/clients/generated/README.md
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 am trying to keep the generated clients updated for all these legacy resources that were created before that change. No reason why, other than just consistency. For all future direct controllers, will not generate the clients. Is it ok to update the generated client in this case, even though it isn't used?
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 understand the consistency concern. But the way to generate these code introduces bug to our code base (and we won't fix them). For direct resources, the lib is already in ./apis
. So this is just adding functionally duplicated, but buggy code.
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.
After offline discussion, I now recognize these generated libs are for customer use, and we said they are no longer updated. Reverted these changes.
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.
/approve
one nit, otherwise looks good!
[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 |
c47dba4
to
b071a45
Compare
No description provided.