-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Devbox daemon #5709
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
base: main
Are you sure you want to change the base?
Devbox daemon #5709
Conversation
Whoa! Easy there, Partner!This PR is too big. Please break it up into smaller PRs. |
TestGru AssignmentSummary
Tip You can |
logger.Info( | ||
"Devbox is shutdown, pod is deleted, commit devbox and create a new content id and new record", | ||
"devboxName", devboxName) | ||
devbox.Status.CommitRecords[devbox.Status.ContentID].CommitStatus = devboxv1alpha1.CommitStatusSuccess |
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.
Should CommitStatusSuccess
be set after the commit actually being done successfully?
// Check if this is a devbox pod by looking for the devbox label, and the devbox is scheduled to the current node | ||
if !r.isDevboxPod(pod) || r.getDevboxNameFromPod(pod) == "" || pod.Spec.NodeName != nodes.GetNodeName() { | ||
return ctrl.Result{}, nil | ||
} |
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.
This can also be rewritten as filters for pod CRUD events
// schedule devbox to node, update devbox status and create a new commit record | ||
// and filter out the devbox that are not in the current node | ||
if devbox.Status.CommitRecords[devbox.Status.ContentID].Node == "" { | ||
// set up devbox node and content id, new a record for the devbox |
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.
TODO: scheduling logic?
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 left some suggestions in the code comments
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5709 +/- ##
=======================================
Coverage 61.97% 61.97%
=======================================
Files 8 8
Lines 647 647
=======================================
Hits 401 401
Misses 200 200
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.