-
Notifications
You must be signed in to change notification settings - Fork 5
Implement Cortex support for Pods #406
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
Conversation
5222b14 to
eb91413
Compare
|
Hey Charly 👋 I noticed your test workflow is failing when trying to post the coverage comment. This happens because PRs from external contributors don't have write permissions to post comments (which is a GitHub security feature). I've fixed this by adding Just rebase on |
f06a397 to
88ca16f
Compare
| // FIXME: is this correct? | ||
| return pod.Spec.SchedulerName == c.Conf.Operator |
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.
Did you check it? For me, it looks good since you set the scheduler name to cortex-pods in the sample file, which matches the configured operator name :D So I think you can remove this comment.
Or is this FIXME related to Arno proposing just cortex as the scheduler name in #392 ?
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.
Oh, I forget to remove the comment. I removed it now.
I also changed the scheduler name from cortex-pods to cortex to conform with Arno's proposal.
I've just tested it and the scheduler name gets set correctly for the test-pod.
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 entirely sure myself whether cortex alone makes sense here, or if cortex-pods would be more precise. However, in a Kubernetes context, cortex should be unique either way.
Did you receive a roadmap from Arno on how to proceed with this feature and its integration into Cortex?
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 did not receive any additional information other than Arno's public issue #392 and the kanban boarder with the next TODOs: https://github.com/orgs/cobaltcore-dev/projects/19/views/1
PhilippMatthes
left a comment
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.
Good work!
Are the CRD definitions necessary? Please remove them.
Otherwise please just bump the dependency and we are good to go
Ref-Issue: #392