Skip to content
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: allow users to specify shipment resource ID #188

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

DavyJ0nes
Copy link

This makes the API more declarative and adherant to the following AIP:
https://google.aip.dev/122#resource-id-segments

@DavyJ0nes DavyJ0nes requested a review from a team September 25, 2023 07:57
@DavyJ0nes DavyJ0nes self-assigned this Sep 25, 2023
Comment on lines 225 to 237
// A user specified ID for the shipment resource. This must adhere to these rules:
// 1. Must only contain upper-case letters, lower-case letters, numbers 0-9 and a hyphen '-'.
// 2. Must have a length between 4 and 63 characters.
// 3. Must start with a letter or a number.
// 4. Must end with a letter or a number.
// The following regular expression can be used to validate these rules:
// ^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$
string shipment_id = 3 [(google.api.field_behavior) = OPTIONAL];

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe add to the description that a random value will be generated if not provided? Read something similar in some google APIs like:
https://github.com/googleapis/google-cloud-go/blob/aiplatform/v1.50.0/aiplatform/apiv1/aiplatformpb/metadata_service.pb.go#L481-L501

Copy link
Author

Choose a reason for hiding this comment

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

Good call, I've updated and also tried made it clear that it must be unique within a space. Also added expected format for resource names similar to the service you linked :)

Copy link
Contributor

@m4mm4r m4mm4r left a comment

Choose a reason for hiding this comment

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

LGTM just one comment on the comment. But OK to go ahead

@DavyJ0nes DavyJ0nes force-pushed the allow-shipment_id-to-be-set-on-request branch 3 times, most recently from a1671c5 to 88d1fe9 Compare September 26, 2023 07:18
This makes the API more declarative and adherant to the following AIP:
https://google.aip.dev/122#resource-id-segments
@DavyJ0nes DavyJ0nes force-pushed the allow-shipment_id-to-be-set-on-request branch from 88d1fe9 to e63856c Compare September 26, 2023 07:21
@DavyJ0nes DavyJ0nes merged commit ec475c2 into master Sep 26, 2023
1 check passed
@DavyJ0nes DavyJ0nes deleted the allow-shipment_id-to-be-set-on-request branch September 26, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants