Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add direct controller for BigQueryDataset #2814

Merged
merged 1 commit into from
Nov 6, 2024
Merged

feat: add direct controller for BigQueryDataset #2814

merged 1 commit into from
Nov 6, 2024

Conversation

xiaoweim
Copy link
Collaborator

@xiaoweim xiaoweim commented Sep 28, 2024 •
edited
Loading

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch 2 times, most recently from 379711a to 2ae68e0 Compare October 1, 2024 19:13
@xiaoweim xiaoweim changed the title feat: add direct controller for BigQueryDataset [WIP] feat: add direct controller for BigQueryDataset Oct 1, 2024
@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from 2ae68e0 to cbc5dd9 Compare October 1, 2024 22:05
@xiaoweim xiaoweim changed the title [WIP] feat: add direct controller for BigQueryDataset feat: add direct controller for BigQueryDataset Oct 1, 2024
@xiaoweim
Copy link
Collaborator Author

xiaoweim commented Oct 1, 2024

/assign @justinsb

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from cbc5dd9 to 4d3534e Compare October 2, 2024 00:06
@xiaoweim
Copy link
Collaborator Author

xiaoweim commented Oct 8, 2024

/assign @yuwenma

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch 7 times, most recently from 69ff85e to fd74bfb Compare October 16, 2024 03:18
@yuwenma
Copy link
Collaborator

yuwenma commented Oct 17, 2024

Good job! Overall looks good and I think the main question is about why you need to resolve project from the seflLink. Besides, there are a few improvements and potential bug fixes that you can just apply the latest controller-template to pick up.

@xiaoweim
Copy link
Collaborator Author

xiaoweim commented Oct 18, 2024 •
edited
Loading

Good job! Overall looks good and I think the main question is about why you need to resolve project from the seflLink. Besides, there are a few improvements and potential bug fixes that you can just apply the latest controller-template to pick up.

Regarding our discussion on why the resolveProjectRef function did not give error, the reason is that when the adapterForObject function is called, the spec.projectRef is already set to the right project. It is not nil so it did not return error.

The reason why the projectRef is already set is because KCC does project defaulting. Here when we create the harness, we set the default project to the test harness. And then we apply the defaults and project annotation

This is where it gets the default project https://github.com/GoogleCloudPlatform/k8s-config-connector/blob/master/pkg/test/gcp/gcp.go#L95. I tried changing the default project in my local config and then it would retunr error.

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch 3 times, most recently from 9766768 to 6f8910e Compare October 18, 2024 17:58
@yuwenma
Copy link
Collaborator

yuwenma commented Oct 18, 2024

This is where it gets the default project master/pkg/test/gcp/gcp.go#L95. I tried changing the default project in my local config and then it would retunr error.

ok, this could relate to the project defaulting https://cloud.google.com/config-connector/docs/how-to/organizing-resources/project-scoped-resources. In such case, the project is not required (but location is)

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from ba0732b to 5e761ba Compare October 25, 2024 21:29
@google-oss-prow google-oss-prow bot removed the lgtm label Oct 25, 2024
@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch 5 times, most recently from 0339614 to 02cca79 Compare October 28, 2024 21:31
@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch 3 times, most recently from dcc47be to 0cc0284 Compare October 31, 2024 06:02
@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from 0cc0284 to d65f0de Compare November 1, 2024 22:43
jingyih
jingyih reviewed Nov 4, 2024
View reviewed changes
Copy link
Collaborator

@jingyih jingyih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a offline discussion with @yuwenma. The controller is currently using the deprecated API for BigQuery datasets. We will need to update the controller and mapping functions to use the new API in follow-up PRs.

pkg/controller/direct/bigquerydataset/dataset_controller.go Show resolved Hide resolved
pkg/controller/direct/bigquerydataset/dataset_controller.go Show resolved Hide resolved
pkg/controller/direct/bigquerydataset/dataset_controller.go Outdated Show resolved Hide resolved
@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from d65f0de to 365271b Compare November 5, 2024 02:09
jingyih
jingyih reviewed Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@jingyih jingyih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

mockgcp/mockbigquery/datasets.go Show resolved Hide resolved
@google-oss-prow google-oss-prow bot added the lgtm label Nov 5, 2024
@jingyih
Copy link
Collaborator

jingyih commented Nov 5, 2024 •
edited
Loading

Defer to @yuwenma for another pass and final approval.

Note that with this PR, the default controller for BigQueryDataset is still TF-based. Users need to opt-in the direct controller using the annotation.

@xiaoweim xiaoweim force-pushed the add_direct_controller_bigquerydataset branch from 365271b to 4764ad9 Compare November 5, 2024 23:10
@google-oss-prow google-oss-prow bot removed the lgtm label Nov 5, 2024
yuwenma
yuwenma reviewed Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@yuwenma yuwenma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@google-oss-prow Google OSS Prow
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jingyih
Copy link
Collaborator

jingyih commented Nov 6, 2024

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 6, 2024
@xiaoweim
Copy link
Collaborator Author

xiaoweim commented Nov 6, 2024

/unhold

@google-oss-prow google-oss-prow bot merged commit 4fba7df into GoogleCloudPlatform:master Nov 6, 2024
19 checks passed
@jingyih jingyih mentioned this pull request Nov 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maqiuyujoyce maqiuyujoyce maqiuyujoyce left review comments

@jingyih jingyih jingyih left review comments

@yuwenma yuwenma yuwenma left review comments

Assignees

@yuwenma yuwenma

@jingyih jingyih

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xiaoweim @yuwenma @jingyih @maqiuyujoyce @justinsb