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

add arg builder to {set,get,delete}DefaultRetention APIs #935

Conversation

balamurugana
Copy link
Member

No description provided.

api/src/main/java/io/minio/MinioClient.java Show resolved Hide resolved
api/src/main/java/io/minio/MinioClient.java Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
MinioClient s3Client =
new MinioClient("https://s3.amazonaws.com", "YOUR-ACCESSKEYID", "YOUR-SECRETACCESSKEY");

ObjectLockConfiguration config =
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should have a complete example, like creating the bucket, setting the default retention and then fetching it. So that user just have to edit the credentials and run the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need to be specific to examples for an API. Its not possible to run any example without modifying essentials like endpoint, credentials, bucket/object names, respective variables etc and some operations are chargeable in some service providers.

We wouldn't consider examples as tests.

@balamurugana balamurugana force-pushed the add-arg-builder-to-set-get-DefaultRetention-APIs branch 5 times, most recently from cb399ba to eb5e515 Compare May 22, 2020 06:57
@balamurugana balamurugana force-pushed the add-arg-builder-to-set-get-DefaultRetention-APIs branch 8 times, most recently from 624ae25 to 762df3c Compare June 2, 2020 07:04
functional/FunctionalTest.java Outdated Show resolved Hide resolved
functional/FunctionalTest.java Outdated Show resolved Hide resolved
} finally {
client.removeBucket(RemoveBucketArgs.builder().bucket(bucketName).build());
}

mintSuccessLog("getDefaultRetention (String bucketName)", null, startTime);

mintSuccessLog(methodName, null, startTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

The retention mode and duration can be logged as arguments

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't be useful because the failure may happen any one of two checks. I am not favour of mint args because stack trace is the best to find the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually if the try-catch and exception handling is also moved to the new method (testGetDefaultRetention) then the args logging can be done.

@balamurugana balamurugana force-pushed the add-arg-builder-to-set-get-DefaultRetention-APIs branch from 762df3c to 98f74f1 Compare June 2, 2020 09:42
@balamurugana balamurugana changed the title add arg builder to {set,get}DefaultRetention APIs add arg builder to {set,get,delete}DefaultRetention APIs Jun 2, 2020
anjalshireesh
anjalshireesh previously approved these changes Jun 2, 2020
Copy link
Contributor

@anjalshireesh anjalshireesh left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment

} finally {
client.removeBucket(RemoveBucketArgs.builder().bucket(bucketName).build());
}

mintSuccessLog("getDefaultRetention (String bucketName)", null, startTime);

mintSuccessLog(methodName, null, startTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually if the try-catch and exception handling is also moved to the new method (testGetDefaultRetention) then the args logging can be done.

Copy link
Contributor

@anjalshireesh anjalshireesh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sinhaashish sinhaashish left a comment

Choose a reason for hiding this comment

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

LGTM

@kannappanr kannappanr merged commit 0b493c1 into minio:master Jun 3, 2020
@balamurugana balamurugana deleted the add-arg-builder-to-set-get-DefaultRetention-APIs branch June 3, 2020 13:12
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.

4 participants