Fix lakectl local commits remote changes outside synced prefix #7796

eladlachmi · 2024-05-22T19:14:40Z

Change Description

Bug Fix

The Problem

Please see the associated issue for the full description of the problem.

Root Cause

lakeFS, unlike Git, doesn't support the concept of committing only staged changes. A commit operation will commit any uncommitted diffs between the refs or fail if conflicts occur (all or nothing).
lakectl local isn't special in this regard. However, being able to sync with any arbitrary prefix creates the expectation that any operations will be scoped to that prefix. In practice, this isn't the case. Merging a ref in lakectl local unexpectedly merges all changes in the remote ref, regardless of prefix.

Solution

This PR adds to lakectl local commit a check for existing uncommitted changes on the remote ref, which are outside of the synced prefix. If such changes are found, the operation is aborted and an informative error message is shown. This behavior can be overridden by adding the the --force flag to the commit command.

Testing Details

These changes were tested manually and with unit tests included in this PR.

Note to reviewers: I believe the included tests cover the possible cases. I would like your feedback if you think that any additional tests are needed for better coverage.

Breaking Change?

This is a breaking change!
Although this behavior is unexpected and considered a bug, it will change following this PR. lakectl local users relying on this behavior will see an error message instead of the operation completed successfully and will need to add the --force flag to get back the previous behavior.

Special thanks to @arielshaqed for the refresher course on char ordinals and string sort order 😅👑

github-actions · 2024-05-22T19:21:05Z

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z · 2024-05-23T06:41:27Z

@eladlachmi I honestly think this issue was mislabled as bug. This is a feature request and regardless it constitutes as a breaking change

N-o-Z

Thanks, really like the approach.
Regarding the tests - I like the unit tests but I would honestly just replace them with a scenario in esti - as we already have tests for lakectl local + commit. I would just add the relevant scenarios there

N-o-Z · 2024-05-23T06:42:44Z

cmd/lakectl/cmd/local_commit.go

+	DieOnErrorOrUnexpectedStatusCode(dirtyResp, err, http.StatusOK)
+
+	// The above gives us SeekGT. Since we need SeekGE, we do another stat for exact match
+	statResp, err := client.StatObjectWithResponse(ctx, remote.Repository, remote.Ref, &apigen.StatObjectParams{


Need to handle err

Very true. Fixed.

N-o-Z · 2024-05-23T06:51:34Z

cmd/lakectl/cmd/local_commit.go

@@ -24,6 +32,47 @@ func findConflicts(changes local.Changes) (conflicts []string) {
 	return
 }

+func hasUncommittedOutsideLocalPrefix(ctx context.Context, client *apigen.ClientWithResponses, remote *uri.URI, idx *local.Index) bool {


A very elegant solution 🎉

I share the credit with @arielshaqed 😁

N-o-Z · 2024-05-23T06:52:55Z

cmd/lakectl/cmd/local_commit.go

@@ -24,6 +32,47 @@ func findConflicts(changes local.Changes) (conflicts []string) {
 	return
 }

+func hasUncommittedOutsideLocalPrefix(ctx context.Context, client *apigen.ClientWithResponses, remote *uri.URI, idx *local.Index) bool {


Suggest something more concise, for example:

Suggested change

func hasUncommittedOutsideLocalPrefix(ctx context.Context, client *apigen.ClientWithResponses, remote *uri.URI, idx *local.Index) bool {

func hasExternalChange(ctx context.Context, client *apigen.ClientWithResponses, remote *uri.URI, idx *local.Index) bool {

As you know, concise is my middle name 😜

N-o-Z · 2024-05-23T07:04:56Z

cmd/lakectl/cmd/local_commit.go

+	}
+
+	// Get the first uncommitted change after the prefix. If it exists, we're also done
+	nextPrefix := fmt.Sprintf("%s%s", filepath.Clean(*currentURI.Path), asciiCharAfterSlash)


I'm not clear what we are doing here. We should optimally get the next prefix in line and perform the diff on that.
I don't understand how the statObject will help, we're assuming some object with some path will exist?
Overall this is very confusing and feels over-complicated

Ok, so I'll explain the approach. Let's say I synced locally with the prefix test-data/.

Case 1: There are no uncommitted changes. The first check doesn't return any results. We're done—continue with the commit.

Case 2: We have uncommitted changes before test-data/, e.g., abc/file.parquet. The first check will return a result outside the prefix. We're done—exit with an error message.

Case 3: We have uncommitted changes after test-data/, e.g., zzz/file.parquet. The second check needs to fetch the first uncommitted change after test-data/. In lexicographic order, this is test-data0 because / is ordinal 47 and the next ordinal (48) is 0. This fetch will return a result-exit with an error message.

These three cases seem to cover all the options. However, there's a subtle edge case. Our After parameter acts like SeekGT. This means that we've covered everything up to test-data0 (end not inclusive) and everything from test-data0 (start not inclusive). So if we happen to have an object specifically named test-data0, we'll miss it. That's what the last StatObject call is for.

Hope this helps clear things up 😊

I get it now 😅
Can we please add the explanation in the code so it will be clear for anyone looking at it?

N-o-Z · 2024-05-23T07:05:27Z

cmd/lakectl/cmd/local_commit.go

+		After:  apiutil.Ptr(apigen.PaginationAfter(nextPrefix)),
+	})
+	DieOnErrorOrUnexpectedStatusCode(dirtyResp, err, http.StatusOK)
+


We can stop here instead of creating an unnecessary API call

See the explanation in the previous comment's reply and let me know what you think.

N-o-Z · 2024-05-23T07:06:15Z

cmd/lakectl/cmd/local_commit.go

+	DieOnErrorOrUnexpectedStatusCode(dirtyResp, err, http.StatusOK)
+
+	// The above gives us SeekGT. Since we need SeekGE, we do another stat for exact match
+	statResp, err := client.StatObjectWithResponse(ctx, remote.Repository, remote.Ref, &apigen.StatObjectParams{


Feels like a shot in the dark

See the explanation in the previous comment's reply and let me know what you think 😅

N-o-Z · 2024-05-23T07:06:43Z

cmd/lakectl/cmd/local_commit.go

@@ -56,6 +106,11 @@ var localCommitCmd = &cobra.Command{
 		resp, err := client.GetBranchWithResponse(cmd.Context(), remote.Repository, remote.Ref)
 		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)

+		hasChangesOutsideSyncedPrefix := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)


Suggested change

hasChangesOutsideSyncedPrefix := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)

hasExternalChange := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)

N-o-Z · 2024-05-23T07:09:46Z

cmd/lakectl/cmd/local_commit.go

@@ -56,6 +106,11 @@ var localCommitCmd = &cobra.Command{
 		resp, err := client.GetBranchWithResponse(cmd.Context(), remote.Repository, remote.Ref)
 		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)

+		hasChangesOutsideSyncedPrefix := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)
+		if hasChangesOutsideSyncedPrefix && !force {
+			DieFmt("The branch you are trying to commit to contains uncommitted changes outside the lakeFS path your local directory '%s' is synced with.\nTo proceed, use the --force flag.", localPath)


You already know the name of the branch:

Suggested change

DieFmt("The branch you are trying to commit to contains uncommitted changes outside the lakeFS path your local directory '%s' is synced with.\nTo proceed, use the --force flag.", localPath)

DieFmt("Branch %s contains uncommitted changes outside of local path '%s'.\nTo proceed, use the --force flag.", remote.ref, localPath)

N-o-Z · 2024-05-23T07:21:57Z

cmd/lakectl/cmd/local_commit.go

@@ -170,6 +225,7 @@ var localCommitCmd = &cobra.Command{

 //nolint:gochecknoinits
 func init() {
+	withForceFlag(localCommitCmd, "Commit changes even if remote branch includes uncommitted changes outside the synced directory")


Suggested change

withForceFlag(localCommitCmd, "Commit changes even if remote branch includes uncommitted changes outside the synced directory")

withForceFlag(localCommitCmd, "Commit changes even if remote branch includes uncommitted changes external to the synced path")

eladlachmi · 2024-05-23T08:54:15Z

@eladlachmi I honestly think this issue was mislabled as bug. This is a feature request and regardless it constitutes as a breaking change

@N-o-Z IMO, any surprising/unexpected behavior hidden from or not clearly communicated to the user is a bug.

I also think we're using the term "breaking change" too loosely. This isn't the same thing as a breaking change in an API.
This is more like a change in the UI.

For example, changing the default checked/unchecked state of a checkbox in a modal is undoubtedly a missable change, and you can and should draw attention clearly to it, at least for the next few versions. However, unlike an API, this is user-interactive, and the option to revert to the previous behavior is just a --force flag away.

I agree that we need to clearly mark this in the release notes as a change lakectl local users need to be aware of, but the term "breaking change" has ramifications, which I don't think apply in this case.

Differing opinions aside, beyond clearly marking and communicating that this is a behavior change, is there anything else you think we need to do before, during, or after releasing this change?

(p.s. to spare the need to add --force each time for users who want this behavior, maybe we can add this as a config option as well? WDYT?)

eladlachmi · 2024-05-23T09:00:53Z

Thanks, really like the approach. Regarding the tests - I like the unit tests but I would honestly just replace them with a scenario in esti - as we already have tests for lakectl local + commit. I would just add the relevant scenarios there

@N-o-Z I don't mind adding esti tests, but since the unit tests are already written, and they provide a localized and fast feedback loop (unlike esti tests), I don't see a reason to remove them. I think a multi-layer testing approach provides good coverage + a good developer experience. WDYT?

N-o-Z · 2024-05-23T09:36:25Z

@eladlachmi I honestly think this issue was mislabled as bug. This is a feature request and regardless it constitutes as a breaking change

@N-o-Z IMO, any surprising/unexpected behavior hidden from or not clearly communicated to the user is a bug.

I also think we're using the term "breaking change" too loosely. This isn't the same thing as a breaking change in an API. This is more like a change in the UI.

For example, changing the default checked/unchecked state of a checkbox in a modal is undoubtedly a missable change, and you can and should draw attention clearly to it, at least for the next few versions. However, unlike an API, this is user-interactive, and the option to revert to the previous behavior is just a --force flag away.

I agree that we need to clearly mark this in the release notes as a change lakectl local users need to be aware of, but the term "breaking change" has ramifications, which I don't think apply in this case.

Differing opinions aside, beyond clearly marking and communicating that this is a behavior change, is there anything else you think we need to do before, during, or after releasing this change?

(p.s. to spare the need to add --force each time for users who want this behavior, maybe we can add this as a config option as well? WDYT?)

Breaking change doesn't necessarily means we need to bump a major version and we shouldn't be afraid to use the term. Simply communicate it appropriately and verify this is understood by all stakeholders (@treeverse/product FYI).
I reject the comparison with UI changes - this is clearly a change in logic. Theoretically users who have used this feature as is and possibly have also relied on the current behavior might find their flows and automation broken. I'm not familiar of any user who automates flow via UI 😵‍💫
Definitely go with the force flag - I see no justification to add it to the configuration

github-actions · 2024-05-23T10:30:23Z

♻️ PR Preview a6e1af6 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

N-o-Z · 2024-05-23T10:37:33Z

Thanks, really like the approach. Regarding the tests - I like the unit tests but I would honestly just replace them with a scenario in esti - as we already have tests for lakectl local + commit. I would just add the relevant scenarios there

@N-o-Z I don't mind adding esti tests, but since the unit tests are already written, and they provide a localized and fast feedback loop (unlike esti tests), I don't see a reason to remove them. I think a multi-layer testing approach provides good coverage + a good developer experience. WDYT?

IMO, mocking the server does not help us identify changes in behavior vs the actual server (extra important since lakectl and lakeFS are different binaries) so we have to have esti test. Once we have this covered in esti anyway, the unit tests become pretty redundant. I don't mind leaving them but I have to insist on having the integration tests

eladlachmi · 2024-05-23T10:40:29Z

I'm not familiar of any user who automates flow via UI 😵‍💫

@N-o-Z How confident are you about your coverage? 😅

Anyway... I think both the CLI and UI are user-interactive tools that users can choose to automate, but in doing so, they should be aware that interfaces (command line or otherwise) don't carry the same guarantees as APIs and SDKs.
Let's agree to disagree 🤝

As long as we agree about what we intend to do, we're good.

N-o-Z

@eladlachmi Thanks for the great work.
Approved with a couple of comments.
It should be clear that this solution is not bulletproof and is susceptible to a race

N-o-Z · 2024-05-26T08:37:07Z

esti/lakectl_local_test.go

+			}
+
+			// add local files
+			if len(tc.uncommittedLocal) > 0 {


I'm not sure but do you need this check?

N-o-Z · 2024-05-26T08:37:49Z

esti/lakectl_local_test.go

@@ -464,6 +464,145 @@ func TestLakectlLocal_commit(t *testing.T) {
 	}
 }

+func TestLakectlLocal_commit_remote_uncommitted(t *testing.T) {


Thanks for adding the tests! 💪🏽

N-o-Z · 2024-05-26T08:39:10Z

cmd/lakectl/cmd/local_commit.go

+	}
+
+	// Get the first uncommitted change after the prefix. If it exists, we're also done
+	nextPrefix := fmt.Sprintf("%s%s", filepath.Clean(*currentURI.Path), asciiCharAfterSlash)


I get it now 😅
Can we please add the explanation in the code so it will be clear for anyone looking at it?

N-o-Z · 2024-05-26T08:41:47Z

cmd/lakectl/cmd/local_commit.go

@@ -56,6 +109,11 @@ var localCommitCmd = &cobra.Command{
 		resp, err := client.GetBranchWithResponse(cmd.Context(), remote.Repository, remote.Ref)
 		DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusOK)

+		hasChangesOutsideSyncedPrefix := hasExternalChange(cmd.Context(), client, remote, idx)


Optimally this should move to line 171 to reduce the chance of a race

eladlachmi added 2 commits May 22, 2024 21:22

Fix lakectl local commits remote changes outside synced prefix

9747421

Revert unintended change

5a37481

eladlachmi added the include-changelog PR description should be included in next release changelog label May 22, 2024

eladlachmi requested review from N-o-Z and arielshaqed May 22, 2024 19:14

eladlachmi self-assigned this May 22, 2024

N-o-Z requested changes May 23, 2024

View reviewed changes

Review fixes

45cb027

eladlachmi requested a review from N-o-Z May 23, 2024 09:59

Add force flag to cli docs

5097f6e

eladlachmi added 6 commits May 23, 2024 17:29

esti tests - draft

4c5b585

esti tests - draft 2

eb1ab47

esti tests - draft 3

3c3c19f

Added esti tests

21a55e3

Add RunCmdAndVerifyFailureContainsText test util func

f1c08de

Added esti test w/ --force

a6e1af6

N-o-Z approved these changes May 26, 2024

View reviewed changes

eladlachmi merged commit bf963e3 into master May 26, 2024
36 checks passed

eladlachmi deleted the 7687-lakectl-local-commits-remote-changes-outside-prefix branch May 26, 2024 08:51

eladlachmi mentioned this pull request May 26, 2024

Additional review fixes #7802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lakectl local commits remote changes outside synced prefix #7796

Fix lakectl local commits remote changes outside synced prefix #7796

eladlachmi commented May 22, 2024

github-actions bot commented May 22, 2024

N-o-Z commented May 23, 2024

N-o-Z left a comment

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 26, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

N-o-Z May 23, 2024

eladlachmi May 23, 2024

eladlachmi commented May 23, 2024

eladlachmi commented May 23, 2024

N-o-Z commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading

N-o-Z commented May 23, 2024

eladlachmi commented May 23, 2024

N-o-Z left a comment

N-o-Z May 26, 2024

N-o-Z May 26, 2024

N-o-Z May 26, 2024

N-o-Z May 26, 2024

	func hasUncommittedOutsideLocalPrefix(ctx context.Context, client apigen.ClientWithResponses, remote uri.URI, idx *local.Index) bool {
	func hasExternalChange(ctx context.Context, client apigen.ClientWithResponses, remote uri.URI, idx *local.Index) bool {

	hasChangesOutsideSyncedPrefix := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)
	hasExternalChange := hasUncommittedOutsideLocalPrefix(cmd.Context(), client, remote, idx)

	DieFmt("The branch you are trying to commit to contains uncommitted changes outside the lakeFS path your local directory '%s' is synced with.\nTo proceed, use the --force flag.", localPath)
	DieFmt("Branch %s contains uncommitted changes outside of local path '%s'.\nTo proceed, use the --force flag.", remote.ref, localPath)

	withForceFlag(localCommitCmd, "Commit changes even if remote branch includes uncommitted changes outside the synced directory")
	withForceFlag(localCommitCmd, "Commit changes even if remote branch includes uncommitted changes external to the synced path")

Fix lakectl local commits remote changes outside synced prefix #7796

Fix lakectl local commits remote changes outside synced prefix #7796

Conversation

eladlachmi commented May 22, 2024

Change Description

Bug Fix

The Problem

Root Cause

Solution

Testing Details

Breaking Change?

github-actions bot commented May 22, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z commented May 23, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladlachmi commented May 23, 2024

eladlachmi commented May 23, 2024

N-o-Z commented May 23, 2024 • edited Loading

github-actions bot commented May 23, 2024 • edited Loading

N-o-Z commented May 23, 2024

eladlachmi commented May 23, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024 •

edited

Loading