From 418fc9f5ff332f903c2969c66d5adfb3fa30e57f Mon Sep 17 00:00:00 2001 From: "Ariel Shaqed (Scolnicov)" Date: Mon, 6 Jan 2025 20:08:07 +0200 Subject: [PATCH] Add "squash merge" support to merge API (#8464) * Add "squash merge" support to merge APIgen Squashing really just means removing the source branch as a parent of the resulting commit. Fixes #7382. * Test "squash merges" API * Recommend in OpenAPI docs to add the source commit to other fields * [CR] State that "squash" is more like Git{Hub,Lab} than like Git * [CR] Add "--merge" option to "lakectl merge" * [CR] Test lakectl with "--merge" option to "lakectl merge" --- api/swagger.yml | 10 ++- clients/java/api/openapi.yaml | 10 +++ clients/java/docs/Merge.md | 1 + .../io/lakefs/clients/sdk/model/Merge.java | 32 ++++++++- .../lakefs/clients/sdk/model/MergeTest.java | 8 +++ clients/python/docs/Merge.md | 1 + clients/python/lakefs_sdk/models/merge.py | 6 +- clients/python/test/test_merge.py | 3 +- clients/rust/docs/Merge.md | 1 + clients/rust/src/models/merge.rs | 4 ++ cmd/lakectl/cmd/merge.go | 13 ++-- docs/assets/js/swagger.yml | 10 ++- docs/reference/cli.md | 1 + .../lakectl_merge_with_squashed_commit.golden | 15 ++++ esti/lakectl_test.go | 68 ++++++++++++++---- pkg/api/controller.go | 1 + pkg/api/controller_test.go | 71 +++++++++++++++++++ pkg/graveler/graveler.go | 15 +++- 18 files changed, 242 insertions(+), 28 deletions(-) create mode 100644 esti/golden/lakectl_merge_with_squashed_commit.golden diff --git a/api/swagger.yml b/api/swagger.yml index 5aaa58b71ef..e907a275f10 100644 --- a/api/swagger.yml +++ b/api/swagger.yml @@ -686,7 +686,15 @@ components: type: boolean default: false description: Allow merge when the branches have the same content - + squash_merge: + type: boolean + default: true + description: | + If set, set only the destination branch as a parent, which "squashes" the merge to + appear as a single commit on the destination branch. The source commit is no longer + a part of the merge commit; consider adding it to the 'metadata' or 'message' + fields. This behaves like a GitHub or GitLab "squash merge", or in Git terms 'git + merge --squash; git commit ...'. BranchCreation: type: object required: diff --git a/clients/java/api/openapi.yaml b/clients/java/api/openapi.yaml index 8b98f319e76..4746a45a4ef 100644 --- a/clients/java/api/openapi.yaml +++ b/clients/java/api/openapi.yaml @@ -8219,6 +8219,7 @@ components: example: metadata: key: metadata + squash_merge: true force: false message: message strategy: strategy @@ -8245,6 +8246,15 @@ components: default: false description: Allow merge when the branches have the same content type: boolean + squash_merge: + default: true + description: | + If set, set only the destination branch as a parent, which "squashes" the merge to + appear as a single commit on the destination branch. The source commit is no longer + a part of the merge commit; consider adding it to the 'metadata' or 'message' + fields. This behaves like a GitHub or GitLab "squash merge", or in Git terms 'git + merge --squash; git commit ...'. + type: boolean type: object BranchCreation: example: diff --git a/clients/java/docs/Merge.md b/clients/java/docs/Merge.md index 7c7a6d108d8..84ccf843486 100644 --- a/clients/java/docs/Merge.md +++ b/clients/java/docs/Merge.md @@ -12,6 +12,7 @@ |**strategy** | **String** | In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch ('dest-wins') or from the source branch('source-wins'). In case no selection is made, the merge process will fail in case of a conflict | [optional] | |**force** | **Boolean** | Allow merge into a read-only branch or into a branch with the same content | [optional] | |**allowEmpty** | **Boolean** | Allow merge when the branches have the same content | [optional] | +|**squashMerge** | **Boolean** | If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. | [optional] | diff --git a/clients/java/src/main/java/io/lakefs/clients/sdk/model/Merge.java b/clients/java/src/main/java/io/lakefs/clients/sdk/model/Merge.java index 2c380740f60..80246e1eb24 100644 --- a/clients/java/src/main/java/io/lakefs/clients/sdk/model/Merge.java +++ b/clients/java/src/main/java/io/lakefs/clients/sdk/model/Merge.java @@ -74,6 +74,10 @@ public class Merge { @SerializedName(SERIALIZED_NAME_ALLOW_EMPTY) private Boolean allowEmpty = false; + public static final String SERIALIZED_NAME_SQUASH_MERGE = "squash_merge"; + @SerializedName(SERIALIZED_NAME_SQUASH_MERGE) + private Boolean squashMerge = true; + public Merge() { } @@ -189,6 +193,27 @@ public void setAllowEmpty(Boolean allowEmpty) { this.allowEmpty = allowEmpty; } + + public Merge squashMerge(Boolean squashMerge) { + + this.squashMerge = squashMerge; + return this; + } + + /** + * If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. + * @return squashMerge + **/ + @javax.annotation.Nullable + public Boolean getSquashMerge() { + return squashMerge; + } + + + public void setSquashMerge(Boolean squashMerge) { + this.squashMerge = squashMerge; + } + /** * A container for additional, undeclared properties. * This is a holder for any undeclared properties as specified with @@ -248,13 +273,14 @@ public boolean equals(Object o) { Objects.equals(this.metadata, merge.metadata) && Objects.equals(this.strategy, merge.strategy) && Objects.equals(this.force, merge.force) && - Objects.equals(this.allowEmpty, merge.allowEmpty)&& + Objects.equals(this.allowEmpty, merge.allowEmpty) && + Objects.equals(this.squashMerge, merge.squashMerge)&& Objects.equals(this.additionalProperties, merge.additionalProperties); } @Override public int hashCode() { - return Objects.hash(message, metadata, strategy, force, allowEmpty, additionalProperties); + return Objects.hash(message, metadata, strategy, force, allowEmpty, squashMerge, additionalProperties); } @Override @@ -266,6 +292,7 @@ public String toString() { sb.append(" strategy: ").append(toIndentedString(strategy)).append("\n"); sb.append(" force: ").append(toIndentedString(force)).append("\n"); sb.append(" allowEmpty: ").append(toIndentedString(allowEmpty)).append("\n"); + sb.append(" squashMerge: ").append(toIndentedString(squashMerge)).append("\n"); sb.append(" additionalProperties: ").append(toIndentedString(additionalProperties)).append("\n"); sb.append("}"); return sb.toString(); @@ -294,6 +321,7 @@ private String toIndentedString(Object o) { openapiFields.add("strategy"); openapiFields.add("force"); openapiFields.add("allow_empty"); + openapiFields.add("squash_merge"); // a set of required properties/fields (JSON key names) openapiRequiredFields = new HashSet(); diff --git a/clients/java/src/test/java/io/lakefs/clients/sdk/model/MergeTest.java b/clients/java/src/test/java/io/lakefs/clients/sdk/model/MergeTest.java index 72b19d25a0e..dfcd5b36171 100644 --- a/clients/java/src/test/java/io/lakefs/clients/sdk/model/MergeTest.java +++ b/clients/java/src/test/java/io/lakefs/clients/sdk/model/MergeTest.java @@ -79,4 +79,12 @@ public void allowEmptyTest() { // TODO: test allowEmpty } + /** + * Test the property 'squashMerge' + */ + @Test + public void squashMergeTest() { + // TODO: test squashMerge + } + } diff --git a/clients/python/docs/Merge.md b/clients/python/docs/Merge.md index 6b0e40f1083..ade085981d9 100644 --- a/clients/python/docs/Merge.md +++ b/clients/python/docs/Merge.md @@ -10,6 +10,7 @@ Name | Type | Description | Notes **strategy** | **str** | In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch ('dest-wins') or from the source branch('source-wins'). In case no selection is made, the merge process will fail in case of a conflict | [optional] **force** | **bool** | Allow merge into a read-only branch or into a branch with the same content | [optional] [default to False] **allow_empty** | **bool** | Allow merge when the branches have the same content | [optional] [default to False] +**squash_merge** | **bool** | If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. | [optional] [default to True] ## Example diff --git a/clients/python/lakefs_sdk/models/merge.py b/clients/python/lakefs_sdk/models/merge.py index 274f42988b3..1e6ffd323d7 100644 --- a/clients/python/lakefs_sdk/models/merge.py +++ b/clients/python/lakefs_sdk/models/merge.py @@ -34,7 +34,8 @@ class Merge(BaseModel): strategy: Optional[StrictStr] = Field(None, description="In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch ('dest-wins') or from the source branch('source-wins'). In case no selection is made, the merge process will fail in case of a conflict") force: Optional[StrictBool] = Field(False, description="Allow merge into a read-only branch or into a branch with the same content") allow_empty: Optional[StrictBool] = Field(False, description="Allow merge when the branches have the same content") - __properties = ["message", "metadata", "strategy", "force", "allow_empty"] + squash_merge: Optional[StrictBool] = Field(True, description="If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. ") + __properties = ["message", "metadata", "strategy", "force", "allow_empty", "squash_merge"] class Config: """Pydantic configuration""" @@ -76,7 +77,8 @@ def from_dict(cls, obj: dict) -> Merge: "metadata": obj.get("metadata"), "strategy": obj.get("strategy"), "force": obj.get("force") if obj.get("force") is not None else False, - "allow_empty": obj.get("allow_empty") if obj.get("allow_empty") is not None else False + "allow_empty": obj.get("allow_empty") if obj.get("allow_empty") is not None else False, + "squash_merge": obj.get("squash_merge") if obj.get("squash_merge") is not None else True }) return _obj diff --git a/clients/python/test/test_merge.py b/clients/python/test/test_merge.py index d22970adb56..206559b31f9 100644 --- a/clients/python/test/test_merge.py +++ b/clients/python/test/test_merge.py @@ -45,7 +45,8 @@ def make_instance(self, include_optional): }, strategy = '', force = True, - allow_empty = True + allow_empty = True, + squash_merge = True ) else : return Merge( diff --git a/clients/rust/docs/Merge.md b/clients/rust/docs/Merge.md index a51ace6df16..de9efa6b6ff 100644 --- a/clients/rust/docs/Merge.md +++ b/clients/rust/docs/Merge.md @@ -9,6 +9,7 @@ Name | Type | Description | Notes **strategy** | Option<**String**> | In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch ('dest-wins') or from the source branch('source-wins'). In case no selection is made, the merge process will fail in case of a conflict | [optional] **force** | Option<**bool**> | Allow merge into a read-only branch or into a branch with the same content | [optional][default to false] **allow_empty** | Option<**bool**> | Allow merge when the branches have the same content | [optional][default to false] +**squash_merge** | Option<**bool**> | If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. | [optional][default to true] [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/clients/rust/src/models/merge.rs b/clients/rust/src/models/merge.rs index 8dedae1f09b..490bedde59f 100644 --- a/clients/rust/src/models/merge.rs +++ b/clients/rust/src/models/merge.rs @@ -25,6 +25,9 @@ pub struct Merge { /// Allow merge when the branches have the same content #[serde(rename = "allow_empty", skip_serializing_if = "Option::is_none")] pub allow_empty: Option, + /// If set, set only the destination branch as a parent, which \"squashes\" the merge to appear as a single commit on the destination branch. The source commit is no longer a part of the merge commit; consider adding it to the 'metadata' or 'message' fields. This behaves like a GitHub or GitLab \"squash merge\", or in Git terms 'git merge --squash; git commit ...'. + #[serde(rename = "squash_merge", skip_serializing_if = "Option::is_none")] + pub squash_merge: Option, } impl Merge { @@ -35,6 +38,7 @@ impl Merge { strategy: None, force: None, allow_empty: None, + squash_merge: None, } } } diff --git a/cmd/lakectl/cmd/merge.go b/cmd/lakectl/cmd/merge.go index 473a842c787..805c1be3b26 100644 --- a/cmd/lakectl/cmd/merge.go +++ b/cmd/lakectl/cmd/merge.go @@ -42,6 +42,7 @@ var mergeCmd = &cobra.Command{ strategy := Must(cmd.Flags().GetString("strategy")) force := Must(cmd.Flags().GetBool("force")) allowEmpty := Must(cmd.Flags().GetBool("allow-empty")) + squash := Must(cmd.Flags().GetBool("squash")) fmt.Println("Source:", sourceRef) fmt.Println("Destination:", destinationRef) @@ -54,11 +55,12 @@ var mergeCmd = &cobra.Command{ } body := apigen.MergeIntoBranchJSONRequestBody{ - Message: &message, - Metadata: &apigen.Merge_Metadata{AdditionalProperties: kvPairs}, - Strategy: &strategy, - Force: &force, - AllowEmpty: &allowEmpty, + Message: &message, + Metadata: &apigen.Merge_Metadata{AdditionalProperties: kvPairs}, + Strategy: &strategy, + Force: &force, + AllowEmpty: &allowEmpty, + SquashMerge: &squash, } resp, err := client.MergeIntoBranchWithResponse(cmd.Context(), destinationRef.Repository, sourceRef.Ref, destinationRef.Ref, body) @@ -89,6 +91,7 @@ func init() { flags.String("strategy", "", "In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch (\"dest-wins\") or from the source branch(\"source-wins\"). In case no selection is made, the merge process will fail in case of a conflict") flags.Bool("force", false, "Allow merge into a read-only branch or into a branch with the same content") flags.Bool("allow-empty", false, "Allow merge when the branches have the same content") + flags.Bool("squash", false, "Squash all changes from source into a single commit on destination") withCommitFlags(mergeCmd, true) rootCmd.AddCommand(mergeCmd) } diff --git a/docs/assets/js/swagger.yml b/docs/assets/js/swagger.yml index 5aaa58b71ef..e907a275f10 100644 --- a/docs/assets/js/swagger.yml +++ b/docs/assets/js/swagger.yml @@ -686,7 +686,15 @@ components: type: boolean default: false description: Allow merge when the branches have the same content - + squash_merge: + type: boolean + default: true + description: | + If set, set only the destination branch as a parent, which "squashes" the merge to + appear as a single commit on the destination branch. The source commit is no longer + a part of the merge commit; consider adding it to the 'metadata' or 'message' + fields. This behaves like a GitHub or GitLab "squash merge", or in Git terms 'git + merge --squash; git commit ...'. BranchCreation: type: object required: diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 3c23ec50751..f1e44fa4813 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2384,6 +2384,7 @@ lakectl merge [flags] -h, --help help for merge -m, --message string commit message --meta strings key value pair in the form of key=value + --squash Squash all changes from source into a single commit on destination --strategy string In case of a merge conflict, this option will force the merge process to automatically favor changes from the dest branch ("dest-wins") or from the source branch("source-wins"). In case no selection is made, the merge process will fail in case of a conflict ``` diff --git a/esti/golden/lakectl_merge_with_squashed_commit.golden b/esti/golden/lakectl_merge_with_squashed_commit.golden new file mode 100644 index 00000000000..17f08396e5e --- /dev/null +++ b/esti/golden/lakectl_merge_with_squashed_commit.golden @@ -0,0 +1,15 @@ + +ID: +Author: esti +Date: