feat(storage): mark preserve_acl as deprecated

[emar-kar]

Closes: #9482

As was asked in the comments, argument preserve_acl marked as deprecated.

[tseaver]

This isn't "deprecation": we are changing the behavior, and emitting a warning. If we are going to deprecate the argument, then we should preserve the existing behavior, and emit an explicit deprecation, e.g.:

        if preserve_acl is not None:
            warnings.warn(
                "The 'preserve_acl' argument is deprecated.  For forward compatibility, "
                 "please update the blob's ACL manually.",
                 DeprecationWarning,
                 stacklevel=2,
            )
        if not preserve_acl and preserve_acl is not None:
            new_blob.acl.save(acl={}, client=client)

[frankyn]

If we do that then, the warning will always be emitted. I think the change makes sense. Otherwise we can do a soft deprecation and only mark it in documentation.

[tseaver]

Please don't assert the warning message (it also happens to be incorrect, as the ACL is for the blob, not its bucket).

[tseaver]

ISTM that we could make the preserve_acl argument actually work, rather than deprecating it, by passing the ACL in the body of the objects.copy request, as suggested in the docs.

[frankyn]

@tseaver just closing the loop, as noted in the reference issue, let's not copy the ACL for the user. Instead it would be better for the user to supply blob metadata that can be used in the copy request. It's a clear opt-in.

Let's deprecate this field and instead look at introducing acl= parameter a user can set in a subsequent PR?

[tseaver]

@frankyn Seems like we should provide a mechanism for copying any properties into the new blob. Maybe the most elegant would be to add a new method to Blob, copy_from. The usage would be:

source = source_bucket.blob('source-name')
dest = dest_bucket.blob('dest-name')
dest.content_type = source.content_type
# Copy other properties here?
dest.copy_from(source, copy_acl=True)

[frankyn]

This exact question came up in Ruby, short update:

The GCS API will copy a subset of metadata from the object on copy or rewrite (excluding ACL's).
As long as the body doesn't include metadata for the object then GCS copies the metadata.
One issue is if the user wants to provide metadata for the copied object, then GCS will not copy the source metadata.

I like the example:

source = source_bucket.blob('source-name')
dest = dest_bucket.blob('dest-name')
dest.content_type = source.content_type
# User Modifies ACLs and copy_from only uses source for its media. 
dest.copy_from(source) 
# no additional parameters here "unless" the user wants to copy over existing metadata in the same way GCS does. We did this in Ruby using a flag force_metadata_copy to respect GCS behavior when a resource is provided. Otherwise, only the metadata provided on the copy/rewrite will be kept. 

[emar-kar]

@frankyn So, the question is that we need probably separate those tasks. First, deprecate preserve_acl. In this question, I'm voting for the deprecation warning. I think users need to see that this argument is not affecting ACL of the new blob to avoid misunderstandings and incorrect behavior. Secondly, open a FR issue to add a new method to copy source data.

[plamut]

Please reopen in the new repo, if this PR is still on track, thanks!