Add "allow-empty" flag to Merge #7798

itaigilo · 2024-05-23T06:40:17Z

Closes #7797.

Change Description

Add allow_no_changes flag to branch merge API, to allow merge merging two branches with the same content.

Background

Currently, when trying to merge two branches with the same content, an "no changes" error is thrown.

New Feature

Add the flag to the API,
And implement support for it in lakectl and in the python-wrapper (in addition to the auto-generated clients).

This change should be fully backwards-compatible.

github-actions · 2024-05-23T06:46:52Z

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z · 2024-05-23T07:01:30Z

@itaigilo You've added the parameter in swagger but did not add the logic in lakeFS.
What will be the default behavior? Is it going to be different from the current behavior? I thought the original bug was that we were able to perform a merge when there were not changes.
I feel like this PR is too small, I'd like to see the usage of the parameter before review

itaigilo · 2024-05-23T07:12:57Z

@N-o-Z this is only a tiny draft, meant to outline the suggested change in the API (before implementing it).
It's basically instead of a spec (according to @arielshaqed 's suggestion) -
The implementation will follow soon.

N-o-Z · 2024-05-23T07:17:41Z

@N-o-Z as noted in Slack, this is only a tiny draft, meant to outline the suggested change in the API (before implementing it). It's basically instead of a spec (according to @arielshaqed 's suggestion) - The implementation will follow soon.

Thanks, IMHO I think the in this instance it's redundant.
The changes themselves in the server are minor, and providing the entire context even as draft is a relatively small effort and provides much more value

arielshaqed

Thanks!

It's great that you sent this PR out for a round of comments. Please also post a link to it on lakeFS Slack #dev channel: The People Must Know, and they may have opinions...

Only required change is to document what "force" will do once we already have this flag.

arielshaqed · 2024-05-25T14:54:25Z

api/swagger.yml

@@ -645,6 +645,10 @@ components:
        force:
          type: boolean
          default: false
+        allow_no_changes:


I guess we will need to document the flag above, too! In particular, what happens if I set force but not allow_no_changes? In order to keep the minor version guarantee, I think that the merge has to fail. This will not be clear to users not aware of the history we are now creating.

So I insist on documenting what force does (sorry, it should have been done before), and I think that it should be documented as not allowing an empty merge.

I believe that this flag should be called allow_empty. It is already called allow_empty for commit and for revert. I don't want to learn another term, and I don't like boolean flags whose names are negative.

github-actions · 2024-05-27T09:22:49Z

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

_{🤖 By surge-preview}

itaigilo · 2024-05-28T08:09:22Z

pkg/api/controller_test.go

+		Message: apiutil.Ptr("Merge branch2 to branch1"),
+	})
+	testutil.MustDo(t, "perform merge with no changes", err)
+	if mergeResp.JSON400 == nil || !strings.HasSuffix(mergeResp.JSON400.Message, graveler.ErrNoChanges.Error()) {


Is there a better way to check this error message?

Unfortunately not for a test at the controller level, this is more of a component test than a unit test. You could unit-test the exact error in TestGravelerMerge (in graveler_v2_test.go, "v2" for Historical Reasons). But I'm not sure it's worth the effort.

arielshaqed

Thanks! This is clear and tested code.

Neat that the majority of changed lines are just a cleanup.

arielshaqed · 2024-05-28T18:13:26Z

pkg/api/controller_test.go

+		Message: apiutil.Ptr("Merge branch2 to branch1"),
+	})
+	testutil.MustDo(t, "perform merge with no changes", err)
+	if mergeResp.JSON400 == nil || !strings.HasSuffix(mergeResp.JSON400.Message, graveler.ErrNoChanges.Error()) {


Unfortunately not for a test at the controller level, this is more of a component test than a unit test. You could unit-test the exact error in TestGravelerMerge (in graveler_v2_test.go, "v2" for Historical Reasons). But I'm not sure it's worth the effort.

N-o-Z

Blocking for the following questions:

What is the current behavior when trying to merge a branch with no changes?
What will be the default behavior after the change?

cleared in description

N-o-Z · 2024-05-28T18:22:28Z

Blocking for the following questions:

What is the current behavior when trying to merge a branch with no changes?

What will be the default behavior after the change?

Noticed it was answered in the updated description

Add allow_no_changes flag to swagger.yml

7d29e49

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

itaigilo self-assigned this May 23, 2024

itaigilo marked this pull request as draft May 23, 2024 06:40

itaigilo changed the title ~~Add allow_no_changes flag to swagger.yml~~ [WIP] Add allow_no_changes flag to swagger.yml May 23, 2024

arielshaqed requested changes May 25, 2024

View reviewed changes

Add AllowEmpty option to the controller and graveler

a30cc10

itaigilo added 3 commits May 28, 2024 10:38

Update force flag description

07f760a

Support flags in lakectl

b0b85e9

Update unit test

f1a1a5d

itaigilo commented May 28, 2024

View reviewed changes

itaigilo marked this pull request as ready for review May 28, 2024 08:10

itaigilo requested a review from arielshaqed May 28, 2024 08:10

Refactor SetOptions creation

cd22acd

itaigilo changed the title ~~[WIP] Add allow_no_changes flag to swagger.yml~~ Add allow_no_changes flag to swagger.yml May 28, 2024

itaigilo changed the title ~~Add allow_no_changes flag to swagger.yml~~ Add "allow-empty" flag to Merge May 28, 2024

Merge branch 'master' into feature/allow-merge-with-no-changes

50540c1

arielshaqed approved these changes May 28, 2024

View reviewed changes

N-o-Z previously requested changes May 28, 2024

View reviewed changes

itaigilo merged commit c0b0d60 into master May 29, 2024
37 checks passed

itaigilo deleted the feature/allow-merge-with-no-changes branch May 29, 2024 14:39

itaigilo mentioned this pull request Jun 2, 2024

lakeFS allows merging a branch to itself #7824

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "allow-empty" flag to Merge #7798

Add "allow-empty" flag to Merge #7798

itaigilo commented May 23, 2024

github-actions bot commented May 23, 2024

N-o-Z commented May 23, 2024

itaigilo commented May 23, 2024 •

edited

Loading

N-o-Z commented May 23, 2024

arielshaqed left a comment

arielshaqed May 25, 2024

github-actions bot commented May 27, 2024 •

edited

Loading

itaigilo May 28, 2024

arielshaqed May 28, 2024

arielshaqed left a comment

arielshaqed May 28, 2024

N-o-Z left a comment

N-o-Z commented May 28, 2024

Add "allow-empty" flag to Merge #7798

Add "allow-empty" flag to Merge #7798

Conversation

itaigilo commented May 23, 2024

Change Description

Background

New Feature

github-actions bot commented May 23, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z commented May 23, 2024

itaigilo commented May 23, 2024 • edited Loading

N-o-Z commented May 23, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed May 25, 2024

Choose a reason for hiding this comment

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

itaigilo May 28, 2024

Choose a reason for hiding this comment

arielshaqed May 28, 2024

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed May 28, 2024

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

N-o-Z commented May 28, 2024

itaigilo commented May 23, 2024 •

edited

Loading

github-actions bot commented May 27, 2024 •

edited

Loading