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

Update awscli tests to log error messages #153

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

nitisht
Copy link
Contributor

@nitisht nitisht commented Sep 26, 2017

Fixes #150

if [ "$HASH_1_MB" == "$hash2" ]; then
function="delete_bucket"
out=$(delete_bucket "$bucket_name")
rv=$?
# remove download file
rm -rf /tmp/datafile-1-MB
Copy link
Member

Choose a reason for hiding this comment

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

Just rm -f is better -r is recursive and we can avoid recursive if not needed..

else
rv=1
out="Downloaded file verfication failed"
Copy link
Member

Choose a reason for hiding this comment

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

Verification failed for downloaded object

@@ -354,6 +366,7 @@ function test_copy_object() {
rv=$?
else
rv=1
out="Copied file verfication failed"
Copy link
Member

Choose a reason for hiding this comment

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

Verification failed for copied object

@@ -158,6 +161,7 @@ function test_create_object() {
rm -rf /tmp/datafile-1-MB
else
rv=1
out="Created object hash verification failed"
Copy link
Member

Choose a reason for hiding this comment

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

Checksum verification failed for uploaded object

@@ -158,6 +161,7 @@ function test_create_object() {
rm -rf /tmp/datafile-1-MB
Copy link
Member

Choose a reason for hiding this comment

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

s/test_create_object/test_upload_object/

@nitisht nitisht force-pushed the awscli-error-log branch 2 times, most recently from faeff5b to 9e4454a Compare September 27, 2017 17:17
@nitisht
Copy link
Contributor Author

nitisht commented Sep 27, 2017

updated @harshavardhana

Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

  1. Can we remove the /tmp/multipart file once it is used?
  2. The buckets are not deleted after a failure in the code, since there is no exception, we should try to clean it up.
  3. Also, the error field should be used for stack trace alone, so, do you think we should change the error field here to message
  4. If AWS returns an error output, they need to be converted to a single line?

Here is an example

{"name": "awscli", "duration": "623", "function": "aws --endpoint-url http://127.0.0.1:9000 s3 sync  s3://awscli-mint-test-bucket-6887/\n", "status": "FAIL", "error": "usage: aws [options] <command> <subcommand> [<subcommand> ...] [parameters]
To see help text, you can run:

aws help
aws <command> help
aws <command> <subcommand> help
aws: error: the following arguments are required: paths"}
  1. Should we log the entire awscli command here in the case of function ? Will it be useful to search if the function contains actual endpoint information, bucket names etc? Or can we simplify the function value to just what we are trying to do here, that way it is consistent between endpoint A and endpoint B.

@nitisht
Copy link
Contributor Author

nitisht commented Sep 28, 2017

  1. Can we remove the /tmp/multipart file once it is used?

Done

  1. The buckets are not deleted after a failure in the code, since there is no exception, we should try to clean it up.

Added bucket removal logic to error logging cases.

  1. Also, the error field should be used for stack trace alone, so, do you think we should change the error field here to message

Removed few of the cases. But in cases where awscli itself passes (i.e. returns exit code 0) and still the test case fails (say object is downloaded but hash verification fails) I dont see other option, as there is no stack to be published and yet we need to tell in the log what caused the test case to fail.

  1. If AWS returns an error output, they need to be converted to a single line?

done

  1. Should we log the entire awscli command here in the case of function ? Will it be useful to search if the function contains actual endpoint information, bucket names etc? Or can we simplify the function value to just what we are trying to do here, that way it is consistent between endpoint A and endpoint B.

I understand this will be taken care based on the discussion we had during today's call.

@kannappanr
Copy link
Collaborator

@balamurugana found one more issue with this log. The duration is a number and it should not have double quotes around the log.

@nitisht
Copy link
Contributor Author

nitisht commented Sep 28, 2017

The duration is a number and it should not have double quotes around the log.

Fixed

Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

Modifying the function name and adding args in the log will be taken up in a different PR. Otherwise LGTM.

@deekoder deekoder merged commit e8f6c93 into minio:master Sep 28, 2017
@nitisht nitisht deleted the awscli-error-log branch January 22, 2018 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants