-
Notifications
You must be signed in to change notification settings - Fork 26
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
Shorten UpdateNodeLabelsAndRecordEvents function in nmc_reconciler.go #850
Shorten UpdateNodeLabelsAndRecordEvents function in nmc_reconciler.go #850
Conversation
Hi @TomerNewman. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-kmm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
3e328b3
to
f017dde
Compare
/assign @yevgeny-shnaidman |
f017dde
to
158c79a
Compare
/ok-to-test |
It looks good to me , but i have another suggestion: |
so it should be in a different package? maybe in |
No, it can be in the same package, for example like PodManager interface |
I'm on it, give me thumbs up to the comment to confirm and I'll push the changes |
I think that the parts that add/remove labels from the nodes should be part of the new For other cases, I definitely think we should create another interface and I would also somehow take into consideration |
I thought about kmmlabels, but its main purpose is to define the labels, to to manipulate what and when should go to wihat node |
158c79a
to
0436d6e
Compare
0436d6e
to
86852c8
Compare
8601713
to
24f27dc
Compare
680e260
to
db98806
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
- Coverage 79.09% 71.95% -7.14%
==========================================
Files 51 66 +15
Lines 5109 4522 -587
==========================================
- Hits 4041 3254 -787
- Misses 882 1073 +191
- Partials 186 195 +9 ☔ View full report in Codecov by Sentry. |
b5eaa80
to
2ed533a
Compare
) | ||
|
||
_, err := r.Reconcile(ctx, req) | ||
_, err = r.Reconcile(ctx, req) | ||
Expect(err).ToNot(BeNil()) |
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 comment is for purpose learning only as it is not directly related to your PR.
This is not a great validation. The description of the spec is should complete all the reconcile functions and return combined error
, therefore, the validation should be to (first return different error messages and then) validate that the combined error contain each one of the error messages of the individual errors.
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.
@ybettan so in that case, I can add validations that each expected error should be in err
, to add it to the PR?
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.
You don't have to add it to the PR so we can merge it faster. You can fix it in a future PR if you like. But to answer your question, the answer is YES.
One way to check it is like this example:
Expect(err.Error()).To(ContainSubstring("failed to get pull options for image")) |
Today, the UpdateNodeLabelsAndRecordEvents function has around 100 lines of code. This commit aims to enhance its readability and maintainability by using helper functions, thereby reducing its total length. Additionally, adding unit tests that covers the new helper functions.
2ed533a
to
fe5d5d8
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman, ybettan 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 |
/lgtm |
Summary
Today, the UpdateNodeLabelsAndRecordEvents function has around 100 lines of code. This commit aims to enhance its readability and maintainability by using helper functions, thereby reducing its total length. Additionally, adding unit tests that covers the new helper functions.