Skip to content

Compare spec with existing dataset, and only perform update if needed#87

Merged
mortenlj merged 3 commits into
mainfrom
mutable-cascading-delete
Jun 8, 2026
Merged

Compare spec with existing dataset, and only perform update if needed#87
mortenlj merged 3 commits into
mainfrom
mutable-cascading-delete

Conversation

@mortenlj

@mortenlj mortenlj commented Jun 8, 2026

Copy link
Copy Markdown
Member

This will allow the user to change cascadingDelete without triggering updates to the actual datasets in Google.

This will allow the user to change `cascadingDelete` without triggering updates to the actual datasets in Google.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds “no-op” update detection to the BigQueryDataset reconciler so that spec-only changes (notably cascadingDelete) don’t trigger a BigQuery Dataset Update call in GCP, while still updating the Kubernetes status hash.

Changes:

  • Skip the GCP Update call when the desired dataset metadata matches the existing dataset metadata (metadataEqual/accessSetEqual).
  • Improve controller tests and mocks to validate that CascadingDelete-only changes do not call Update, and add unit tests for the equality helpers.
  • Add/adjust repo maintenance files (agent instructions, ignore rules, mise task dependency).

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
mise.toml Updates task dependencies for local builds (and needs a follow-up fix for local:run).
controllers/suite_test.go Makes the BigQuery mock thread-safe and adds update call counting.
controllers/bigquerydataset_controller.go Adds metadata equality checks to avoid unnecessary BigQuery Update calls.
controllers/bigquerydataset_controller_test.go Adds tests ensuring cascadingDelete toggles don’t trigger GCP updates; adds unit tests for equality helpers.
AGENTS.md Documents contributor/agent workflow and project conventions.
.gitignore Ignores additional local files/dirs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mise.toml
Comment thread controllers/bigquerydataset_controller_test.go Outdated
Comment thread controllers/bigquerydataset_controller.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

mise.toml:82

  • local:run still depends on a vet task, but only check:vet is defined in this mise.toml. As written, mise run local:run will fail with an unknown task dependency.
depends = ["generate", "fmt", "check:vet"]
run = "go build -o bin/manager main.go"

[tasks."local:run"]
description = "Run a controller from your host"

if !accessSetEqual(computedAccess, existing.Access) {
return false
}
if existing.Labels["team"] != dataset.GetNamespace() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Vi sjekker om de har app, men tror ikke det er noen garanti at de har satt team.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Controlleren setter team label både i onCreate og onUpdate, så den burde være der. Og hvis ikke, så er det kanskje riktig at en update trengs? Dette er label på datasettet i Google, ikke ressursen i clusteret ...

@mortenlj mortenlj merged commit e6e3b3e into main Jun 8, 2026
1 check passed
@mortenlj mortenlj deleted the mutable-cascading-delete branch June 8, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants