-
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
🌱 Add lifecycle hook types #6537
🌱 Add lifecycle hook types #6537
Conversation
/hold Until #5141 is merged |
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.
There's an outstanding question in #6506
About whether we should compose the types by creating shared structs used throughout the top-level types.
I'm happy to do that in this PR if that's the direction we're looking to go.
As discussed yesterday, let's finalize #6506 and then depending on timing potentially update this PR to embed shared structs in the response types (alternatively we can do it in a follow-up PR). |
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.
One nit otherwise lgtm from my side
/hold cancel |
/hold |
Let's keep this on hold until #6499 is merged. There may be changes that I'd prefer to do once. |
115473c
to
bb8bb76
Compare
/hold cancel |
9db0ba9
to
dd3e6c8
Compare
dd3e6c8
to
94949a3
Compare
Thank you! /assign @fabriziopandini |
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.
Overall LGTM.
Just a few minor nits. Not blocking.
94949a3
to
4a2f90e
Compare
b299384
to
0773189
Compare
0773189
to
5dd989f
Compare
5dd989f
to
3149e35
Compare
Co-authored-by: ykakarap <[email protected]> Signed-off-by: killianmuldoon <[email protected]>
3149e35
to
62b6184
Compare
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.
just a question, optional though.
/lgtm
/lgtm |
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.
Types are consistent with what was discussed in the proposal, let's keep moving and eventually improve documentation as soon as we can audit the first set of generated OpenAPI specifications
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
Co-authored-by: ykakarap [email protected]
Signed-off-by: killianmuldoon [email protected]
Add types for the lifecycle hooks defined in the lifecycle hook proposal.
Part of #6330
Implements the types from proposal #5141
/area runtime-sdk