-
Notifications
You must be signed in to change notification settings - Fork 238
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
mockgcp: ComputeManagedSSLCertificate #2010
mockgcp: ComputeManagedSSLCertificate #2010
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.
qq: I don't see update method, nor update.yaml test file. Is that intended?
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.
Yes there is no update
/ patch
for this resource AFAICT.
return nil, status.Errorf(codes.Internal, "error creating sslCertificate: %v", err) | ||
} | ||
|
||
return s.newLRO(ctx, name.Project.ID) |
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.
Suggest using s.StartLRO
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.
This was mostly cherry-picked but thanks for pointing that out 💯 we should probably clarify in the guide when to use newLRO
vs startGlobalLRO
|
||
{ | ||
"id": "000000000000000000000", | ||
"insertTime": "2024-04-01T12:34:56.123456Z", |
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.
interesting, do you know the difference between insertTime
and createTime
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.
Not quite certain, maybe it is resource specific?
70f1db6
to
83021d7
Compare
OperationType: PtrTo("delete"), | ||
User: PtrTo("[email protected]"), | ||
} | ||
return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) { |
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.
oh I like this!
/lgtm Thank you! Looks great! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Alex Pana <[email protected]>
Integrates c0b4f8e from GoogleCloudPlatform#936 Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
/lgtm |
8bc7767
into
GoogleCloudPlatform:master
Breaking up from #936 for the mockgcp of
ComputeManagedSSLCertificate
. Did a few integration changes but mostly cherry picking Justin's changes 🙏🏼