-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(bigtable): add GC policy to FamilyInfo. #5993
feat(bigtable): add GC policy to FamilyInfo. #5993
Conversation
bigtable/admin.go
Outdated
GCPolicy string | ||
Name string | ||
GCPolicy string | ||
GCPolicyProto *btapb.GcRule |
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.
Have you considered just adding the fields you need directly on this object, rather than exposing the whole proto?
In general we try to avoid exposing proto-types directly via the manual clients. Though, I see a few cases already where they are in this library, so I'll just have this as a question/suggestion rather than being a purist about it.
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.
We're interested in recursively iterate through this proto to generate a valid JSON field for Terraform. The one we're interested in is the Rule object in *btapb.GcRule, but the type of that object is not exported, so I opt in to pass the entire proto here.
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.
I agree with @tritone. We typically try to provide a veneer over proto message types.
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.
I see, so how about adding a new struct in the bigtable client and a parser function to convert the proto to the new struct, and pass that struct back from the admin?
bigtable/admin.go
Outdated
GCPolicy string | ||
Name string | ||
GCPolicy string | ||
GCPolicyProto *btapb.GcRule |
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.
I agree with @tritone. We typically try to provide a veneer over proto message types.
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.
Does this have a corresponding Issue?
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.
Two more small comments, otherwise LGTM. @telpirion must approve though.
It looks like something similar to this feature is already available in the Java BT client, so it's reasonable to include here. |
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
Pull request was closed
We need the proto of GC policy exposed so that Terraform can read max_version and max_age from the policy, instead of a fully parsed string. Fix #6122.