-
Notifications
You must be signed in to change notification settings - Fork 227
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
[jobframework] Only check if the owner is managed for enabled integrations. #2493
[jobframework] Only check if the owner is managed for enabled integrations. #2493
Conversation
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/test all |
/retest |
/assign |
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.
Implementation lgtm. Question related to improving reliability by an integration level test, which would cover the change in SetupControllers
.
/lgtm |
LGTM label has been added. Git tree hash: 5b8b87cd12688d45b899edfe7a71db8c79d94b35
|
/release-note-edit
|
|
||
// EnableIntegrationsForTest - should be used only in tests | ||
// Mark the frameworks identified by names and return a revert function. | ||
func EnableIntegrationsForTest(tb testing.TB, names ...string) func() { |
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.
Maybe for a follow up, but I would prefer that, instead, we get rid of the manager as a global variable and pass it to the functions or store it in the structs that need it.
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'm not a fan of global states, however in this case when we are doing init bases framework registration this is our best option.
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.
It can still be done differently. There could be one DefaultIntegrationManager
that the implementations can register to.
But, you still can pass a different manager to the objects that need it.
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.
Sure it could have been done in a different way, but it was done like this a long time ago and reviewed at that time.
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, but, at that time, we were not modifying it in tests.
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: ab20814953f829dba2144f872197f1081f81772c
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, trasc 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Only check if the owner of a job is managed against the list of jobs integrations currently enabled .
Which issue(s) this PR fixes:
Fixes #2481
Special notes for your reviewer:
Does this PR introduce a user-facing change?