fix: improve listing #3333
fix: improve listing #3333
Conversation
Signed-off-by: Alex Pana <[email protected]>
pkg/controller/direct/bigqueryanalyticshub/listing_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Alex Pana <[email protected]>
|
cc @yuwenma these small changes make the alpha controller usable for this resource |
| return err | ||
| } | ||
| obj.Spec.Source.BigQueryDatasetSource.DatasetRef.External = id.String() |
There was a problem hiding this comment.
I don't quite follow here. Could you elaborate a little bit more on why using the listing resource ID as its reference resource' external?
We would want to use the datasetRef.NormalizeExternal. I know the dataset is not fully migrated to direct yet, but the NormalizeExternal does not depend on which controller the resource uses.
There was a problem hiding this comment.
I can see the current dataset reference is blocked by the parent https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/apis/bigquery/v1beta1/dataset_reference.go
We can use the new struct and it should be fixed. cc @xiaoweim
There was a problem hiding this comment.
the id on line 100 is the new "old" resolved dataset reference. We need to populate the .External so the mappers function right for this reference of the object.
There was a problem hiding this comment.
ah, ok. Suggest renaming to datasetID in ln100 and tableID in line 108.
Signed-off-by: Alex Pana <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jingyih, 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 |
google-oss-prow
bot
merged commit dee215a
into
GoogleCloudPlatform:master
A few improvements for the listing resource