Skip to content

docs(samples): remove bucket CORS configuration#7288

Merged
frankyn merged 4 commits into
googleapis:masterfrom
athakor:storage_sample_doc
Jul 22, 2020
Merged

docs(samples): remove bucket CORS configuration#7288
frankyn merged 4 commits into
googleapis:masterfrom
athakor:storage_sample_doc

Conversation

@athakor

@athakor athakor commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 8, 2020
@athakor
athakor requested review from JesseLovelace and frankyn July 8, 2020 11:17
@athakor

athakor commented Jul 10, 2020

Copy link
Copy Markdown
Contributor Author

@frankyn PTAL


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does null not work here?

@athakor athakor Jul 20, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@lesv lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@frankyn frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@frankyn
frankyn merged commit cd43d0d into googleapis:master Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants