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

Implement lakectl local commit #6369

Merged
merged 2 commits into from
Aug 13, 2023
Merged

Implement lakectl local commit #6369

merged 2 commits into from
Aug 13, 2023

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Aug 10, 2023

Closes #6306

Change Description

Background

Implement lakectl local command to commit changes from local directory to a linked lakeFS branch

New Feature

Part of task #6239

Testing Details

Unit testing for tools, lakectl tests will be added for esti upon feature completion. SyncManager will be tested as part of the command (was tested manually)

Breaking Change?

No

@N-o-Z N-o-Z added area/lakectl Issues related to lakeFS' command line interface (lakectl) exclude-changelog PR description should not be included in next release changelog lakectl-local labels Aug 10, 2023
@N-o-Z N-o-Z requested a review from guy-har August 10, 2023 18:06
@N-o-Z N-o-Z self-assigned this Aug 10, 2023
@github-actions
Copy link

github-actions bot commented Aug 10, 2023

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

🕐 Build time: 0.024s

🤖 By surge-preview

@N-o-Z N-o-Z force-pushed the task/lakectl-local-commit-6306 branch from ec99d02 to 880f7ad Compare August 10, 2023 19:34
@N-o-Z N-o-Z force-pushed the task/lakectl-local-commit-6306 branch from 880f7ad to db048e8 Compare August 11, 2023 10:08
Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +303 to +305
if resp.StatusCode() != http.StatusNoContent {
return fmt.Errorf("could not delete object: HTTP %d: %w", resp.StatusCode(), helpers.ErrRequestFailed)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if resp.StatusCode() != http.StatusNoContent {
return fmt.Errorf("could not delete object: HTTP %d: %w", resp.StatusCode(), helpers.ErrRequestFailed)
}
if resp.StatusCode() != http.StatusNoContent {
b.Error()
return fmt.Errorf("could not delete object: HTTP %d: %w", resp.StatusCode(), helpers.ErrRequestFailed)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Found a better solution

Comment on lines +20 to +23
err = os.Remove(name)
if err != nil {

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err = os.Remove(name)
if err != nil {
}
_ = os.Remove(name)

@N-o-Z N-o-Z enabled auto-merge (squash) August 13, 2023 09:33
@N-o-Z N-o-Z merged commit 357350e into master Aug 13, 2023
33 checks passed
@N-o-Z N-o-Z deleted the task/lakectl-local-commit-6306 branch August 13, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lakectl Issues related to lakeFS' command line interface (lakectl) exclude-changelog PR description should not be included in next release changelog lakectl-local
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement lakectl local commit
2 participants