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.

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

Added ability for deferred mutator execution #380

Merged
merged 6 commits into from
May 16, 2023

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented May 10, 2023

Changes

Added DeferredMutator and bundle.Defer function which allows to always execute some mutators either in the end of execution chain or after error occurs in the middle of execution chain.

Usage as follows:

deferredMutator := bundle.Defer([]bundle.Mutator{
    lock.Acquire()
    transform.DoSomething(),
    //...
}, []bundle.Mutator{
    lock.Release(),
})

In such case lock.Release() will always be executed: either when all operations above succeed or when any of them fails

Tests

Before the change

andrew.nester@HFW9Y94129 multiples-tasks % bricks bundle deploy
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/simple-task/development/files!

Error: terraform not initialized
andrew.nester@HFW9Y94129 multiples-tasks % bricks bundle deploy
Error: deploy lock acquired by andrew.nester@databricks.com at 2023-05-10 16:41:22.902659 +0200 CEST. Use --force to override

After the change

andrew.nester@HFW9Y94129 multiples-tasks % bricks bundle deploy 
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/simple-task/development/files!

Error: terraform not initialized
andrew.nester@HFW9Y94129 multiples-tasks % bricks bundle deploy
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/simple-task/development/files!

Error: terraform not initialized

@andrewnester andrewnester requested review from pietern and a team May 10, 2023 15:05
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

bundle/deferred.go Outdated Show resolved Hide resolved
bundle/deferred.go Outdated Show resolved Hide resolved

func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) {
mainErr := Apply(ctx, b, d.mutators)
errOnFinish := Apply(ctx, b, d.onFinishOrError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Existing mutators return nested mutators instead of executing them in place. I like this better, tbh, so once this is merged we should change the prototype of the mutator interface to no longer return []Mutator and make all mutators execute nested mutators in place.

terraform.StatePush(),
}, []bundle.Mutator{
lock.Release(),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This makes me think we also need (separate PR of course) a bundle.Seq to execute multiple mutators in sequence such that we can compose lock.Acquire with a bundle.Defer where the unlock happens. As it is here, lock.Release would always be called even if lock.Acquire fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pietern just to clarify to make sure I understand correctly. You propose to have a new method bundle.Seq with API like this

func Seq(mutators []Mutator) Mutator { ... }

seq := bundle.Seq(
		bundle.Defer([]bundle.Mutator{
			files.Upload(),
			artifacts.UploadAll(),
			terraform.Interpolate(),
			terraform.Write(),
			terraform.StatePull(),
			terraform.Apply(),
			terraform.StatePush(),
		}, []bundle.Mutator{
			lock.Release(),
		}))

deployPhase := []bundle.Mutator{
    lock.Acquire(),
    seq,
}

bundle.Seq will essentially transform list of mutators into Mutator object with Apply method so it can be added to the list of mutators

In such case seq will only be executed once lock.Acquire succeeds

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes to the proposed prototype, but the call site would be slightly different and call the Seq function to compose call lock.Acquire and the defer block in sequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So something like this then?

deferred := bundle.Defer([]bundle.Mutator{
			files.Upload(),
			artifacts.UploadAll(),
			terraform.Interpolate(),
			terraform.Write(),
			terraform.StatePull(),
			terraform.Apply(),
			terraform.StatePush(),
		}, []bundle.Mutator{
			lock.Release(),
		})

seq := bundle.Seq([lock.Acquire(), deferred])

deployPhase := []bundle.Mutator{
   seq
}

Do we then really need bundle.Seq? What if bundle.Defer just returns Mutator instead of []Mutator?
In that case we would just have it used like

deployPhase := []bundle.Mutator{
   lock.Acquire(),
   deferred
}

@pietern any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't if we leave everything else the same, so yeah you could leave as is for this PR.

The comment on bundle.Seq is because we have mixed usage of singular Mutator and []Mutator in a couple places and formalizing []Mutator into a bundle.Seq would get rid of that.

terraform.StatePush(),
}, []bundle.Mutator{
lock.Release(),
files.Delete(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This means file deletion will happen even if the user doesn't consent. You could move files.Delete into the main block for the time being. Then when we have a bundle.Seq (see other comment), we can sequence this defer block with a final delete to happen after lock release has completed.

@andrewnester andrewnester requested a review from pietern May 12, 2023 11:58
libs/errors/errors.go Outdated Show resolved Hide resolved
@andrewnester andrewnester merged commit 180dfc9 into main May 16, 2023
4 checks passed
@andrewnester andrewnester deleted the defer-mutator-operations branch May 16, 2023 16:01
@pietern pietern mentioned this pull request May 16, 2023
pietern added a commit that referenced this pull request May 16, 2023
## Changes

This release bumps the minor version to 100 to disambiguate between
Databricks CLI "v1" (the Python version)
and this version, Databricks CLI "v2". This release is a major rewrite
of the CLI, and is not backwards compatible.

CLI:
* Rename bricks -> databricks
([#389](#389)).

Bundles:
* Added ability for deferred mutator execution
([#380](#380)).
* Do not truncate local state file when pulling remote changes
([#382](#382)).
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.

None yet

3 participants