-
Notifications
You must be signed in to change notification settings - Fork 508
minimal integration reference change #4697
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
minimal integration reference change #4697
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kannon92 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
e38b1d4 to
948a671
Compare
|
Actually, out of the two I prefer this one in #4676 for consistently using IntegrationReference throughout the codebase where applicable. My comment was about grouping all the integrations in the monolithic style. Note that, there is no guarantee when adding a new integration to Kueue someone will put it on the new list. It would compile equally well if the constant was declared in the scope of the package for the new integration. I'm open to the possibility, that I'm missing something, but I would like to take a step back and define the problem we are solving. For example, the issue mentions MXJob, which was not in the comments for a good reason, as no longer supported. And I think there is no need to rush, we can do it post 0.11, and post KubeCon :) . /hold |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Use IntegrationReference instead of strings.
Which issue(s) this PR fixes:
Maybe #4659
Special notes for your reviewer:
Does this PR introduce a user-facing change?