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

Merge API response - remove summary field #5115

Merged
merged 1 commit into from
May 31, 2023
Merged

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Jan 26, 2023

This is API breaking change

Remove merge response summary as we do not reflect the actual numbers.
In case caller would like to have data about the merge, it should be reflected by calling diff after getting the new merge reference.

Close #3978

This remains a very far-reaching change. For instance: say I upgrade my lakeFS. Then my Airflow DAGs stop working!

Let's add a note to the CHANGELOG now (could be in the next PR, of course, rather than wait for whoever next releases lakeFS).

Users who use lakeFS SDK lower than v0.91.0 and verify the response will not work with this version of lakeFS as we dropped the merge summary.

@nopcoder nopcoder added area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Jan 26, 2023
@nopcoder nopcoder self-assigned this Jan 26, 2023
@nopcoder nopcoder requested a review from a team January 26, 2023 16:58
@arielshaqed
Copy link
Contributor

Does this break existing code? I would prefer to return junk numbers, or an arbitrary map, than do that.

If we do decide to break b/c, we will need to communicate to our users. We are one of those users, I think our Airflow provider provides merge. It mustn't break.

@nopcoder
Copy link
Contributor Author

It will break clients with old API like other breaking changes. No existing code in lakeFS uses this response.
Will check other integrations, and give heads up before it will become part of lakeFS.

@nopcoder nopcoder changed the title Merge API remove summary from response Merge API response - remove summary field Jan 27, 2023
@nopcoder
Copy link
Contributor Author

@arielshaqed created #5119 to have a version where the field is optional while the server will return a valid answer until we remove the summary field.

@nopcoder nopcoder force-pushed the chore/api-merge-summary branch from b67df37 to a1deb0f Compare February 6, 2023 10:51
@nopcoder nopcoder closed this May 25, 2023
@nopcoder nopcoder force-pushed the chore/api-merge-summary branch from 2db91b0 to 67ed15f Compare May 25, 2023 17:00
@nopcoder nopcoder reopened this May 25, 2023
@github-actions
Copy link

github-actions bot commented May 25, 2023

🎊 PR Preview f37b0a8 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-5115.surge.sh

🕐 Build time: 0.027s

🤖 By surge-preview

@nopcoder nopcoder removed the request for review from a team May 25, 2023 17:17
Comment on lines +1609 to +1613
return "bare"
case sampleData:
ref = "sample"
return "sample"
}
return ref
return ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the use of ref as it also a package name in this code

@nopcoder nopcoder requested a review from arielshaqed May 29, 2023 13:13
@nopcoder nopcoder force-pushed the chore/api-merge-summary branch from 62746d8 to f37b0a8 Compare May 29, 2023 13:14
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

LGTM

@arielshaqed
Copy link
Contributor

This remains a very far-reaching change. For instance: say I upgrade my lakeFS. Then my Airflow DAGs stop working!

Let's add a note to the CHANGELOG now (could be in the next PR, of course, rather than wait for whoever next releases lakeFS).

@nopcoder
Copy link
Contributor Author

This remains a very far-reaching change. For instance: say I upgrade my lakeFS. Then my Airflow DAGs stop working!

Let's add a note to the CHANGELOG now (could be in the next PR, of course, rather than wait for whoever next releases lakeFS).

Thanks @arielshaqed!! I'll fix the client version used by Airflow before merging this one and add a note.

@nopcoder
Copy link
Contributor Author

nopcoder commented May 30, 2023

Update lakefs airflow provider treeverse/airflow-provider-lakeFS#66
Client SDK update wasn't required as the previous version v0.91.0 already had merge results optional.
Create and test using python client v0.91.0 merge with this branch to verify manually that we don't break.

@nopcoder nopcoder merged commit 41384b0 into master May 31, 2023
@nopcoder nopcoder deleted the chore/api-merge-summary branch May 31, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge response contains zero values
3 participants