The Wayback Machine - https://web.archive.org/web/20201207000624/https://github.com/googleapis/google-cloud-java/pull/7288
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

docs(samples): remove bucket CORS configuration #7288

Merged
merged 4 commits into from Jul 22, 2020

Conversation

@athakor
Copy link
Member

@athakor athakor commented Jul 8, 2020

No description provided.

@googlebot googlebot added the cla: yes label Jul 8, 2020
@athakor athakor requested review from frankyn and JesseLovelace Jul 8, 2020
@athakor
Copy link
Member Author

@athakor athakor commented Jul 10, 2020

@frankyn PTAL


Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService();
Bucket bucket = storage.get(bucketName);
bucket.toBuilder().setCors(ImmutableList.<Cors>of()).build().update();

This comment has been minimized.

@frankyn

frankyn Jul 17, 2020
Member

does null not work here?

This comment has been minimized.

@athakor

athakor Jul 20, 2020
Author Member

@frankyn null setting doesn't works here see the respected example with response

        Storage storage = StorageOptions.getDefaultInstance().getService();
        String bucketName = RemoteStorageHelper.generateBucketName();
        try{
            Cors cors = Cors.newBuilder()
                    .setOrigins(ImmutableList.of(Cors.Origin.of("*")))
                    .setMethods(ImmutableList.of(HttpMethod.GET))
                    .setResponseHeaders(ImmutableList.of("Content-Type"))
                    .setMaxAgeSeconds(100)
                    .build();
            Bucket bucket =
                    storage.create(BucketInfo.newBuilder(bucketName).setCors(ImmutableList.of(cors)).build());
            System.out.println("Prior size : " + bucket.getCors().size());
            Bucket updatedBucket  = bucket.toBuilder().setCors(null).build().update();
            System.out.println("After updating operation size must be zero instead of : " + updatedBucket.getCors().size());

        }finally {
            RemoteStorageHelper.forceDelete(storage, bucketName, 5, TimeUnit.SECONDS);
        }

Response :

Prior size : 1
After updating operation size must be zero instead of : 1

This comment has been minimized.

@athakor

athakor Jul 20, 2020
Author Member

@frankyn I have raise the issue in storage repo regarding this and will fixed it soon

This comment has been minimized.

@lesv

lesv Jul 20, 2020
Collaborator

I can see @frankyn 's argument, but following it's point, why need to call setCors() at all if it's empty or NULL?
Lots of folks (Effective Java 3rd ed.) prefer empty lists to using NULL, and I was going to comment w/ that, but really, if it's empty, why have to set it at all?

This comment has been minimized.

@frankyn

frankyn Jul 20, 2020
Member

In this case the example is to remove CORS from a bucket metadata. Good call, I think we can move forward with this PR with ImmutableList.<Cors>Of()

This comment has been minimized.

@frankyn

frankyn Jul 20, 2020
Member

Maybe one additional change that could be made here @athakor is to confirm that CORS are set before making a subsequent call. That way you reduce the need to make a patch request.

This comment has been minimized.

@athakor

athakor Jul 21, 2020
Author Member

done

Copy link
Collaborator

@lesv lesv left a comment

Approved for java-samples-reviewers


Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService();
Bucket bucket = storage.get(bucketName);
bucket.toBuilder().setCors(ImmutableList.<Cors>of()).build().update();

This comment has been minimized.

@lesv

lesv Jul 20, 2020
Collaborator

I can see @frankyn 's argument, but following it's point, why need to call setCors() at all if it's empty or NULL?
Lots of folks (Effective Java 3rd ed.) prefer empty lists to using NULL, and I was going to comment w/ that, but really, if it's empty, why have to set it at all?

Copy link
Member

@frankyn frankyn left a comment

Whoops, missed the second point that @lesv made.


Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService();
Bucket bucket = storage.get(bucketName);
bucket.toBuilder().setCors(ImmutableList.<Cors>of()).build().update();

This comment has been minimized.

@frankyn

frankyn Jul 20, 2020
Member

Maybe one additional change that could be made here @athakor is to confirm that CORS are set before making a subsequent call. That way you reduce the need to make a patch request.

List<Cors> cors = new ArrayList<>(bucket.getCors());

// Remove bucket CORS configuration.
for (int index = 0; index < cors.size(); index++) {

This comment has been minimized.

@frankyn

frankyn Jul 21, 2020
Member

Use cors.clear() instead if all cors will be deleted.

This comment has been minimized.

@athakor

athakor Jul 22, 2020
Author Member

done

@frankyn frankyn merged commit cd43d0d into googleapis:master Jul 22, 2020
7 checks passed
7 checks passed
Kokoro - Test: Code Format Build successful
Details
Kokoro - Test: Java 11 Build successful
Details
Kokoro - Test: Java 7 Build successful
Details
Kokoro - Test: Java 8 Build successful
Details
Kokoro - Test: Linkage Monitor Build successful
Details
Kokoro - Test: Windows Java 8 Build successful
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.