-
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 API types for DiscoverVariables hook #7986
🌱 Add API types for DiscoverVariables hook #7986
Conversation
@@ -22,6 +22,7 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime" | |||
"k8s.io/apimachinery/pkg/types" | |||
|
|||
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" |
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 don't think adding this import is a problem here - we import the same for lifecycle hooks. But I did want to point it out to find out if there's any opinions about defining the variables for VariableDiscovery as their own type - in case changes need to be made down the line etc.
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.
Thx for bringing it up, I think same as for lifecycle hooks it's fine to have this dependency for now. Means we have to evolve them together but that should be okay for now.
b9f2150
to
22dd3d9
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.
/area runtime-sdk
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 looks good.
22dd3d9
to
5dba291
Compare
Thx! /lgtm |
LGTM label has been added. Git tree hash: 0beb08a0baa56950313e514d6b6567ffa471da0a
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
Add the API types proposed for the DiscoverVariables extension to the runtime API package.
Part of #7985