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

fix: ls differentiate exit codes for empty and non-existent buckets #732

Merged
merged 22 commits into from
Jul 10, 2024

Conversation

tarikozyurtt
Copy link
Contributor

@tarikozyurtt tarikozyurtt commented Jul 4, 2024

Previously, s5cmd ls returned an exit code of 1 for both empty buckets and non-existent buckets, making it difficult to differentiate between the two cases. This change updates the behavior to:

  • Return exit code 0 for empty buckets, similar to the behavior of s3cmd.

  • Return exit code 1 for non-existent buckets, providing a clear distinction.

Example behavior after the change:

s5cmd ls s3://empty-bucket returns exit code 0. It does not print anything. s5cmd ls s3://non-existent-bucket returns exit code 1 with an appropriate error message. Also, select command with --all-versions true flag no longer prints ERROR "s3://bucket/": no object found message for empty buckets. Similarly, du command used to print

ERROR "du s3://empty-bucket": no object found
0 bytes in 0 objects: s3://empty-bucket

Now it will omit the part with error, it will yield:

0 bytes in 0 objects: s3://empty-bucket

They both exit with 0 without any change.

Resolves #722

Previously, s5cmd ls returned an exit code of 1 for both empty buckets and non-existent buckets, making it difficult to differentiate between the two cases. This change updates the behavior to:

Return exit code 0 for empty buckets, similar to the behavior of s3cmd.
Return exit code 1 for non-existent buckets, providing a clear distinction.
Example behavior after the change:

s5cmd ls s3://empty-bucket returns exit code 0. It does not print anything.
s5cmd ls s3://non-existent-bucket returns exit code 1 with an appropriate error message.
Also, select command with --all-versions true flag no longer prints "ERROR " s3://bucket/": no object found" message for empty buckets.
Similarly, du command used to print
"ERROR "du s3://empty-bucket": no object found
0 bytes in 0 objects: s3://empty-bucket"
Now it will omit "ERROR "du s3://bucket": no object found" part. They both exits with 0 without any change.
@tarikozyurtt tarikozyurtt requested a review from a team as a code owner July 4, 2024 10:14
@tarikozyurtt tarikozyurtt requested review from igungor and denizsurmeli and removed request for a team July 4, 2024 10:14
@denizsurmeli
Copy link
Contributor

denizsurmeli commented Jul 4, 2024

Hi, thanks for your contribution. It would be nice to extend test cases for demonstrating the behavior you have mentioned in the PR description:

Also, select command with --all-versions true flag no longer prints "ERROR " s3://bucket/": no object found" message for empty buckets. Similarly, du command used to print
"ERROR "du s3://empty-bucket": no object found
0 bytes in 0 objects: s3://empty-bucket"
Now it will omit "ERROR "du s3://bucket": no object found" part. They both exit with 0 without any change.

Adding these assertions as tests and when we run those tests in this revision, they should fail.

Previously, s5cmd ls returned an exit code of 1 for both empty buckets and non-existent buckets, making it difficult to differentiate between the two cases. This change updates the behavior to:

Return exit code 0 for empty buckets, similar to the behavior of s3cmd.
Return exit code 1 for non-existent buckets, providing a clear distinction.
Example behavior after the change:

s5cmd ls s3://empty-bucket returns exit code 0. It does not print anything.
s5cmd ls s3://non-existent-bucket returns exit code 1 with an appropriate error message.
Also, select command with --all-versions true flag no longer prints "ERROR " s3://bucket/": no object found" message for empty buckets.
Similarly, du command used to print
"ERROR "du s3://empty-bucket": no object found
0 bytes in 0 objects: s3://empty-bucket"
Now it will omit "ERROR "du s3://bucket": no object found" part. They both exits with 0 without any change.
e2e/ls_test.go Outdated Show resolved Hide resolved
e2e/ls_test.go Outdated Show resolved Hide resolved
e2e/du_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Show resolved Hide resolved
tarikozyurtt and others added 9 commits July 5, 2024 10:32
Co-authored-by: Deniz Surmeli <52484739+denizsurmeli@users.noreply.github.com>
Co-authored-by: Deniz Surmeli <52484739+denizsurmeli@users.noreply.github.com>
Co-authored-by: Deniz Surmeli <52484739+denizsurmeli@users.noreply.github.com>
* ls: change error code for no object found case

* fix main case for bucket path (tests, other List cases and README remaining)

* add bucket case for all cases

* rm comment lines

* add empty bucket test

* add changes to changelog

* add tests for cmds that use ls
e2e/select_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Outdated Show resolved Hide resolved
e2e/select_test.go Outdated Show resolved Hide resolved
Co-authored-by: Deniz Surmeli <52484739+denizsurmeli@users.noreply.github.com>
denizsurmeli
denizsurmeli previously approved these changes Jul 10, 2024
Copy link
Contributor

@denizsurmeli denizsurmeli left a comment

Choose a reason for hiding this comment

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

FYI: If your comments are in the Pending state, the reviewers do not see it. Thus, some of the reviews might come a bit off, thinking that you have responded/changed already :)

sonmezonur
sonmezonur previously approved these changes Jul 10, 2024
@tarikozyurtt tarikozyurtt dismissed stale reviews from sonmezonur and denizsurmeli via 16244b2 July 10, 2024 08:01
@denizsurmeli denizsurmeli merged commit 21fa2da into peak:master Jul 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on empty bucket listing
4 participants