Skip to content

Conversation

@JesseLovelace
Copy link
Contributor

First attempt at adding to GRPC, includes DeleteHmacKey operation and HmacKeyMetadata modeling + property tests.

Forgive any errors i made with property testing, i'm new here!

@JesseLovelace JesseLovelace requested review from a team as code owners May 13, 2022 22:49
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels May 13, 2022
@JesseLovelace JesseLovelace changed the title Jesse grpc DeleteHmacKey GRPC May 13, 2022
@JesseLovelace JesseLovelace changed the title DeleteHmacKey GRPC feat: DeleteHmacKey and HmacKeyMetadata GRPC May 13, 2022
Copy link
Collaborator

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

A couple minor things, feel free to merge after addressing.

final StringBuilder sb = new StringBuilder();
sb.append(first).append(mid).append(last);
// can only contain lowercase letters
return new ProjectID(sb.toString().toLowerCase());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can actually push the lower case constraint back up to the arbitrary by doing

Arbitraries.chars().range('a', 'z').numeric()

This has the added benefit that the arbitrary is aware of the constraint and can therefore track and reduce against this.

I'll make a similar change to the arbitrary for BucketName in a separate PR.

@Override
public Set<Arbitrary<?>> provideFor(TypeUsage targetType, SubtypeProvider subtypeProvider) {

@UpperChars Arbitrary<String> accessID = Arbitraries.strings().alpha().numeric().ofLength(61);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The annotation here doesn't actually effect this how we'd hope. (The annotations are useful along with the @ForAll to constrain the inputs).

To get the desired set we'd need to do something like:

    Arbitrary<String> accessID = Arbitraries.strings().withCharRange('A', 'Z').numeric().ofLength(61);

@BenWhitehead
Copy link
Collaborator

Thanks for the update @JesseLovelace. Feel free to merge after rebasing

@BenWhitehead BenWhitehead merged commit ad6ded7 into feat/grpc-storage May 19, 2022
@BenWhitehead BenWhitehead deleted the jesse-grpc branch May 19, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the googleapis/java-storage API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants