-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: kueue limitation to job #260
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR implements Kueue functionality by adding configuration parameters and Kubernetes resources. The implementation includes defining a ClusterQueue, LocalQueue, and ResourceFlavor using Kueue's API, with configurable concurrent job limits and queue names. Class diagram for Kueue configurationclassDiagram
class Kueue {
+bool enabled
+string fullnameOverride
+QueueName queueName
+int concurrentJobs
}
class QueueName {
+string cluster
+string local
}
Kueue --> QueueName
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ambersun1234 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider publishing the Kueue chart to a proper chart repository instead of using a local file reference. This would improve maintainability and version management.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
67fb032
to
1b8f18f
Compare
1b8f18f
to
0cc34b8
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.
Hey @ambersun1234 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider changing the default
kueue.enabled
tofalse
to make this feature opt-in rather than opt-out - There's a typo in the parameter name 'cocurrentJobs' - should be 'concurrentJobs'
- Please add documentation explaining the purpose of Kueue integration and guidelines for configuring the parameters (especially concurrentJobs)
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0cc34b8
to
c32f32f
Compare
charts/agh3/values.yaml
Outdated
## @section Kueue parameters | ||
kueue: | ||
## @param kueue.enabled Enable internal kueue | ||
enabled: true |
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 should be disabled by default until all refactoring is complete.
c32f32f
to
4a67a5d
Compare
ebaf8e8
to
c8b2829
Compare
c8b2829
to
e80183f
Compare
need to merge #273 before converting to ready |
e80183f
to
9591268
Compare
9591268
to
1b75084
Compare
Summary by Sourcery
New Features: