-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: Add RPO metadata settings #1105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want someone a bit more versed in Java to take a look but to me this is 👍
@@ -54,34 +54,16 @@ | |||
import com.google.cloud.kms.v1.KeyManagementServiceGrpc.KeyManagementServiceBlockingStub; | |||
import com.google.cloud.kms.v1.KeyRingName; | |||
import com.google.cloud.kms.v1.LocationName; | |||
import com.google.cloud.storage.Acl; | |||
import com.google.cloud.storage.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intellij does this automatically and i can't figure out how to turn it off it's infuriating 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-cloud-storage/src/main/java/com/google/cloud/storage/Rpo.java
Outdated
Show resolved
Hide resolved
@@ -21,6 +21,11 @@ | |||
<method>com.google.cloud.storage.BucketInfo$Builder deleteLifecycleRules()</method> | |||
<differenceType>7013</differenceType> | |||
</difference> | |||
<difference> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenWhitehead we hit this each time we make a change to a public interface, we've been told it's to protect us from breaking changes, however we are committing breaking changes. It has gone on for years. What are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clirr is in place to primarily help us know if we are committing a change that breaks binary compatibility. The place it's most beneficial is on our generated code and our "main" apis. In this case, it's less beneficial and unfortunately BucketInfo.Builder is abstract and not annotated with @InternalExtensionOnly
which would give us leeway to add methods with no guarantee of binary compatibility for out of project implementations. However, adding this annotation is itself considered a breaking change and would need to be performed at a major version.
Our policy for model classes in general has been that we consider them to generally be safe for adding new methods, unfortunately clirr isn't capable of being made aware that this is a model class and instead requires an entry defined. Unfortunately, at this point, I think we're sort of stuck having to deal with this file until either we reach a new major version when we can annotate this builder and define a general rule or we decide to refactor things and make this class not be abstract and instead simply non-final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM unless @BenWhitehead has any additional comments!
google-cloud-storage/src/main/java/com/google/cloud/storage/Rpo.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/Rpo.java
Outdated
Show resolved
Hide resolved
@@ -21,6 +21,11 @@ | |||
<method>com.google.cloud.storage.BucketInfo$Builder deleteLifecycleRules()</method> | |||
<differenceType>7013</differenceType> | |||
</difference> | |||
<difference> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clirr is in place to primarily help us know if we are committing a change that breaks binary compatibility. The place it's most beneficial is on our generated code and our "main" apis. In this case, it's less beneficial and unfortunately BucketInfo.Builder is abstract and not annotated with @InternalExtensionOnly
which would give us leeway to add methods with no guarantee of binary compatibility for out of project implementations. However, adding this annotation is itself considered a breaking change and would need to be performed at a major version.
Our policy for model classes in general has been that we consider them to generally be safe for adding new methods, unfortunately clirr isn't capable of being made aware that this is a model class and instead requires an entry defined. Unfortunately, at this point, I think we're sort of stuck having to deal with this file until either we reach a new major version when we can annotate this builder and define a general rule or we decide to refactor things and make this class not be abstract and instead simply non-final.
Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>
Adds RPO metadata settings for buckets.
Holding off on adding documentation until this general implementation is approved
See googleapis/google-cloud-java#7700
Fixes: #1106