-
Notifications
You must be signed in to change notification settings - Fork 39
Un-exporting some of WorkerPodManager interface env variables.
#1025
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ybettan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubernetes-sigs-kmm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1025 +/- ##
==========================================
- Coverage 79.09% 73.07% -6.03%
==========================================
Files 51 72 +21
Lines 5109 6076 +967
==========================================
+ Hits 4041 4440 +399
- Misses 882 1455 +573
+ Partials 186 181 -5 ☔ View full report in Codecov by Sentry. |
internal/pod/workerpodmanager.go
Outdated
| ListWorkerPodsOnNode(ctx context.Context, nodeName string) ([]v1.Pod, error) | ||
| LoaderPodTemplate(ctx context.Context, nmc client.Object, nms *kmmv1beta1.NodeModuleSpec) (*v1.Pod, error) | ||
| UnloaderPodTemplate(ctx context.Context, nmc client.Object, nms *kmmv1beta1.NodeModuleStatus) (*v1.Pod, error) | ||
| IsLoaderPod(p v1.Pod) bool |
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 think all those function can receive a Pod pointer, instead of a Pod structure. Passing it as a struct mean copying it, and it is a large structure. There are a lot of functions , even in kuberenetes packages, that receive parameters as pointers, event though they are not changing them
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.
Fixed.
When this interface was transitioned to the `pod` package, it was copied as is (as much as possible) and some variables had to be exported to be used as before. Once the transition done, it was simpler to review some of those new exported env variables and convert them to usefull methods inside the `WorkerPodManager` interface. Signed-off-by: Yoni Bettan <[email protected]>
dd56c9f to
8125ed7
Compare
|
/lgtm |
Bumps [k8s.io/kubectl](https://github.com/kubernetes/kubectl) from 0.29.0 to 0.29.2. - [Commits](kubernetes/kubectl@v0.29.0...v0.29.2) --- updated-dependencies: - dependency-name: k8s.io/kubectl dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
When this interface was transitioned to the
podpackage, it was copied as is (as much as possible) and some variables had to be exported to be used as before.Once the transition done, it was simpler to review some of those new exported env variables and convert them to usefull methods inside the
WorkerPodManagerinterface./assign @yevgeny-shnaidman