Skip to content

direct: Detect remote changes#3666

Merged
denik merged 2 commits intomainfrom
denik/remote-diff1
Sep 30, 2025
Merged

direct: Detect remote changes#3666
denik merged 2 commits intomainfrom
denik/remote-diff1

Conversation

@denik
Copy link
Contributor

@denik denik commented Sep 26, 2025 •
edited
Loading

Changes

Calculate diff of snapshot state with remote state and calculate action based on that.

Why

Remote drift detection for direct.

Tests

New acceptance tests.

@denik denik temporarily deployed to test-trigger-is September 26, 2025 13:05 — with GitHub Actions Inactive
@eng-dev-ecosystem-bot
Copy link
Collaborator

eng-dev-ecosystem-bot commented Sep 26, 2025 •
edited
Loading

Run: 18130343813

Env ✅​pass 🔄​flaky 🙈​skip
✅​ aws linux 315 536
✅​ aws windows 316 535
✅​ aws-ucws linux 428 433
✅​ aws-ucws windows 429 432
✅​ azure linux 315 535
✅​ azure windows 316 534
🔄​ azure-ucws linux 425 3 432
✅​ azure-ucws windows 429 431
✅​ gcp linux 314 537
✅​ gcp windows 315 536
Test Name azure-ucws linux
TestAccept 🔄​flaky
TestAccept/bundle/templates/default-python/integration_classic 🔄​flaky
TestAccept/bundle/templates/default-python/integration_classic/DATABRICKS_BUNDLE_ENGINE=direct-exp/UV_PYTHON=3.10 🔄​flaky

@denik denik changed the title direct: detect remote drift direct: detect remote changes Sep 29, 2025
@denik denik force-pushed the denik/remote-diff1 branch from 4c6ba0b to 8c36084 Compare September 29, 2025 13:08
@denik denik temporarily deployed to test-trigger-is September 29, 2025 13:09 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 29, 2025 13:20 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 29, 2025 13:31 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 29, 2025 14:04 — with GitHub Actions Inactive
@denik denik force-pushed the denik/structvar branch 2 times, most recently from f17eab1 to 7565f79 Compare September 29, 2025 14:39
@denik denik force-pushed the denik/remote-diff1 branch from 9cbd4bb to 9b75154 Compare September 29, 2025 14:41
@denik denik temporarily deployed to test-trigger-is September 29, 2025 14:41 — with GitHub Actions Inactive
@denik denik marked this pull request as ready for review September 29, 2025 14:42
@denik denik force-pushed the denik/remote-diff1 branch from 9b75154 to d914af3 Compare September 29, 2025 15:35
@denik denik temporarily deployed to test-trigger-is September 29, 2025 15:35 — with GitHub Actions Inactive
pietern
pietern reviewed Sep 30, 2025
View reviewed changes
bundle/direct/bundle_plan.go

// We have a choice whether to include remoteState or remoteStateComparable from below.
// Including remoteState because in the near future remoteState is expected to become a superset struct of remoteStateComparable
entry.RemoteState = remoteState
Copy link
Contributor

@pietern pietern Sep 30, 2025

Choose a reason for hiding this comment

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

Let's discuss the implications of this live.

I think we need to use the remapped one to keep the payloads in the plan comparable by other tools.

Copy link
Contributor

@pietern pietern Sep 30, 2025

Choose a reason for hiding this comment

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

Btw, this is also what triggers inclusion of the remote state in the "skip" actions.

Copy link
Contributor Author

@denik denik Sep 30, 2025

Choose a reason for hiding this comment

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

I think we need to use the remapped one to keep the payloads in the plan comparable by other tools.

Yes, superset == comparable.

Note, we also list paths that are compared explicitly.

Btw, this is also what triggers inclusion of the remote state in the "skip" actions.

Not sure what you mean. It's good to have remote state for skip actions, the difference might be explicitly nullified and it's good to see what the remote value was in this case.

Copy link
Contributor

@pietern pietern Sep 30, 2025

Choose a reason for hiding this comment

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

I agree, but in isolation, the remote state isn't that useful. Only if we also include the local state (and opt the previous state) can we see the full picture.

Copy link
Contributor Author

@denik denik Sep 30, 2025

Choose a reason for hiding this comment

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

I agree, but in isolation, the remote state isn't that useful.

What do you mean by "in isolation"? What's missing from the plan? Note, that new_state is omitted when it's the same as previous state (which we will refer to, we already have it in separate file).

Copy link
Contributor Author

@denik denik Sep 30, 2025

Choose a reason for hiding this comment

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

Merging to unblock follow ups, we can tweak this later.

@denik denik force-pushed the denik/remote-diff1 branch from d914af3 to bdf6e86 Compare September 30, 2025 10:28
@denik denik temporarily deployed to test-trigger-is September 30, 2025 10:28 — with GitHub Actions Inactive
@denik denik requested a review from pietern September 30, 2025 10:37
@denik denik temporarily deployed to test-trigger-is September 30, 2025 10:42 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 30, 2025 10:49 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 30, 2025 10:57 — with GitHub Actions Inactive
@pietern pietern changed the title direct: detect remote changes direct: Detect remote changes Sep 30, 2025
Base automatically changed from denik/structvar to main September 30, 2025 12:08
@denik denik force-pushed the denik/remote-diff1 branch from 59bb417 to 0bb2796 Compare September 30, 2025 12:19
@denik
add remote diff
af99623 
wip

move

update test

clean up

update

add jobs_update_remote test

include remote state

update

add 'return false'

update tests after rebase

update tests

fix test

remove json plan - differences due to remote state
@denik denik force-pushed the denik/remote-diff1 branch from 0bb2796 to af99623 Compare September 30, 2025 12:30
@denik denik temporarily deployed to test-trigger-is September 30, 2025 12:30 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is September 30, 2025 12:46 — with GitHub Actions Inactive
@denik denik added this pull request to the merge queue Sep 30, 2025
Merged via the queue into main with commit bf3356b Sep 30, 2025
13 checks passed
@denik denik deleted the denik/remote-diff1 branch September 30, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewnester andrewnester andrewnester approved these changes

@anton-107 anton-107 Awaiting requested review from anton-107 anton-107 is a code owner

@shreyas-goenka shreyas-goenka Awaiting requested review from shreyas-goenka shreyas-goenka is a code owner

@pietern pietern Awaiting requested review from pietern pietern is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@denik @eng-dev-ecosystem-bot @pietern @andrewnester