-
Notifications
You must be signed in to change notification settings - Fork 238
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
promote Bigqueryconnection to v1beta1 API #3100
promote Bigqueryconnection to v1beta1 API #3100
Conversation
@jingyih pls review |
dab77ef
to
409cfa4
Compare
...bigqueryconnection/v1alpha1/bigqueryconnectionconnection/cloudsqlconnectionbasic/create.yaml
Show resolved
Hide resolved
@@ -1188,7 +1188,7 @@ X-Xss-Protection: 0 | |||
|
|||
{ | |||
"cloudSql": { | |||
"database": "sqldatabase-sample-${uniqueId}", | |||
"database": "projects/${projectId}/instances/sqlinstance-sample-${uniqueId}/databases/sqldatabase-sample-${uniqueId}", |
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 unexpected. I don't think this works with real GCP API? My understanding is that the underlying GCP API expects the name of the database.
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 tested it against real GCP API and it turns out the database field can be either just the name of database, or the name in the form project/instance/database
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.
According to the API doc, this field is just the name of db
pkg/controller/direct/bigqueryconnection/connection_controller.go
Outdated
Show resolved
Hide resolved
409cfa4
to
65ee2e7
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.
The only blocking comment is that we need to update the alpha API and regenerate the CRD. Other comments can be addressed in a followup PR.
external: ${PROJECT_ID?} | ||
cloudSpanner: | ||
databaseRef: | ||
name: bigqueryconnection-dep |
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.
nit: name should be bigqueryconnectionconnection-dep
. Same for the other dependencies.
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.
According to the document, the length of name cannot exceed max 15 characters
projectRef: | ||
# Replace ${PROJECT_ID?} with your project ID | ||
external: ${PROJECT_ID?} | ||
spark: {} |
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 we create a "DataprocCluster" for this sample? (Can be a followup PR)
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.
Currently, Mockgcp service for Dataproc has not been implemented. Thus I suggest we add this in a followup PR
@maqiuyujoyce @yuwenma How do we handle the URI skeleton test when we migrate a resource to direct? Currently the unit test panics. k8s-config-connector/pkg/resourceskeleton/testdata/uri-skeleton.yaml Lines 765 to 767 in e12f6d0
|
The files were removed in the previous commit as a result of executing "git mv". Restore the files by checking them out from master HEAD. "git checkout master -- apis/bigqueryconnection/v1alpha1"
rm -rf pkg/clients/generated/client/clientset/versioned/typed/bigqueryconnection/v1alpha1 scripts/generate-go-crd-clients/generate-clients.sh make ready-pr
then run make ready-pr
e0f9fc2
to
3af2766
Compare
/approve |
[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 |
885f4af
into
GoogleCloudPlatform:master
Change description
Promote bigqueryconnection API to v1beta1
make ready-pr
to ensure this PR is ready for review.