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

Correct name for force acquire deploy flag #656

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Correct name for force acquire deploy flag #656

merged 1 commit into from
Aug 15, 2023

Conversation

shreyas-goenka
Copy link
Contributor

Changes

As discussed here, the name for this flag should be force-lock: #578 (comment)

Tests

Manually and existing tests

@shreyas-goenka shreyas-goenka requested review from lennartkats-db, pietern and andrewnester and removed request for lennartkats-db August 11, 2023 11:51
andrewnester
andrewnester previously approved these changes Aug 11, 2023
View reviewed changes
pietern
pietern requested changes Aug 11, 2023
View reviewed changes
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.

--force-lock can be interpreted as actually forcing to use the lock, instead of disregarding it.

@pietern
Copy link
Contributor

pietern commented Aug 11, 2023

Paging @lennartkats-db to chime in on this.

andrewnester
andrewnester reviewed Aug 11, 2023
View reviewed changes
internal/locker_test.go
@@ -90,7 +90,7 @@ func TestAccLock(t *testing.T) {
indexOfAnInactiveLocker = i
}
assert.ErrorContains(t, lockerErrs[i], "lock acquired by")
assert.ErrorContains(t, lockerErrs[i], "Use --force to override")
Copy link
Contributor

Choose a reason for hiding this comment

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

why was it --force and not --force-deploy before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We named it force before recent changes that introduced git validation logic and renamed the flag to --force-deploy for deploy and --force-lock for destroy

@andrewnester andrewnester self-requested a review August 11, 2023 13:55
andrewnester
andrewnester reviewed Aug 11, 2023
View reviewed changes
libs/locker/locker.go
@@ -105,10 +105,10 @@ func (locker *Locker) assertLockHeld(ctx context.Context) error {
return err
}
if activeLockState.ID != locker.State.ID && !activeLockState.IsForced {
return fmt.Errorf("deploy lock acquired by %s at %v. Use --force to override", activeLockState.User, activeLockState.AcquisitionTime)
return fmt.Errorf("deploy lock acquired by %s at %v. Use --force-lock to override", activeLockState.User, activeLockState.AcquisitionTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was it --force and not --force-deploy before?

@andrewnester
Copy link
Contributor

Shall we make it backward compatible change? Otherwise it might break any workflows that currently use this flag

cc @pietern

@shreyas-goenka
Copy link
Contributor Author

For private preview IMO its fine if the change is not backwards compatible since this is not a flag you should use in your workflows anyways.

@lennartkats-db
Copy link
Contributor

So per #578 (comment), @pietern and I previously had a discussion on what to name this flag. We indeed had the concern that Pieter called out above:

@pietern

--force-lock can be interpreted as actually forcing to use the lock, instead of disregarding it.

But we also felt that it might be the most concise and practical name :/

The implementation with --force-deploy is actually a mistake on my part. The intention was to use --force-lock.

@lennartkats-db
Copy link
Contributor

@andrewnester

Shall we make it backward compatible change? Otherwise it might break any workflows that currently use this flag

Making this change now in private preview is fine. But we could change the locking error to hint to use "--force-lock" whenever a customer uses "--lock" and a lock still exists.

@lennartkats-db
Copy link
Contributor

Btw the other change we should still consider is to track the client + process id that last did a deploy. If I do a deploy on my laptop with pid 15151 and hit Control-C then on re-deploy it should just notice that process no longer exists and re-deploy without asking questions.

@pietern
Copy link
Contributor

pietern commented Aug 15, 2023

Thanks for confirming, @lennartkats-db .

This means this PR is making things consistent again, even if we don't like the name of the flag.

@pietern pietern added this pull request to the merge queue Aug 15, 2023
Merged via the queue into main with commit 6a843f2 Aug 15, 2023
4 checks passed
@pietern pietern deleted the fix-force-lock-name branch August 15, 2023 19:11
@pietern pietern mentioned this pull request Aug 16, 2023
Merged
pietern added a commit that referenced this pull request Aug 16, 2023
@pietern
Release v0.203.1
bac00a6 
CLI:
 * Always resolve .databrickscfg file ([#659](#659)).

Bundles:
 * Add internal tag for bundle fields to be skipped from schema ([#636](#636)).
 * Log the bundle root configuration file if applicable ([#657](#657)).
 * Execute paths without the .tmpl extension as templates ([#654](#654)).
 * Enable environment overrides for job clusters ([#658](#658)).
 * Merge artifacts and resources block with overrides enabled ([#660](#660)).
 * Locked terraform binary version to <= 1.5.5 ([#666](#666)).
 * Return better error messages for invalid JSON schema types in templates ([#661](#661)).
 * Use custom prompter for bundle template inputs ([#663](#663)).
 * Add map and pair helper functions for bundle templates ([#665](#665)).
 * Correct name for force acquire deploy flag ([#656](#656)).
 * Confirm that override with a zero value doesn't work ([#669](#669)).

Internal:
 * Consolidate functions in libs/git ([#652](#652)).
 * Upgraded Go version to 1.21 ([#664](#664)).
github-merge-queue bot pushed a commit that referenced this pull request Aug 16, 2023
@pietern
Release v0.203.1 (#672)
35e8ed3 
CLI:
* Always resolve .databrickscfg file
([#659](#659)).

Bundles:
* Add internal tag for bundle fields to be skipped from schema
([#636](#636)).
* Log the bundle root configuration file if applicable
([#657](#657)).
* Execute paths without the .tmpl extension as templates
([#654](#654)).
* Enable environment overrides for job clusters
([#658](#658)).
* Merge artifacts and resources block with overrides enabled
([#660](#660)).
* Locked terraform binary version to <= 1.5.5
([#666](#666)).
* Return better error messages for invalid JSON schema types in
templates ([#661](#661)).
* Use custom prompter for bundle template inputs
([#663](#663)).
* Add map and pair helper functions for bundle templates
([#665](#665)).
* Correct name for force acquire deploy flag
([#656](#656)).
* Confirm that override with a zero value doesn't work
([#669](#669)).

Internal:
* Consolidate functions in libs/git
([#652](#652)).
* Upgraded Go version to 1.21
([#664](#664)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewnester andrewnester andrewnester left review comments

@pietern pietern pietern approved these changes

@lennartkats-db lennartkats-db lennartkats-db approved these changes

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
@shreyas-goenka @pietern @andrewnester @lennartkats-db