feat: retain pod spec containers resources when sync pod reource #311
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 33.06% 33.24% +0.18%
==========================================
Files 155 155
Lines 17876 17892 +16
==========================================
+ Hits 5910 5948 +38
+ Misses 11471 11435 -36
- Partials 495 509 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -470,6 +470,9 @@ func retainContainers(desiredContainers, clusterContainers []interface{}) error | |||
| } | |||
|
|
|||
| func retainContainer(desiredContainer, clusterContainer map[string]interface{}) error { | |||
| resources, _, _ := unstructured.NestedFieldNoCopy(clusterContainer, "resources") | |||
There was a problem hiding this comment.
What if NestedFieldNoCopy returns error or !exists? Pls explicitly handle boundary cases.
There was a problem hiding this comment.
This field is a non-pointer struct, and during parsing, it will be considered as existing whether it is empty or has a value. Therefore, I did not add this check. I have observed similar situations in other code as well. Please also help me check if it is necessary to add it.
There was a problem hiding this comment.
When I constructed an empty map, the unit test did fail. So I added the necessary check.
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "nil retain test", |
There was a problem hiding this comment.
| name: "nil retain test", | |
| name: "retain nil resources", |
| }, | ||
| }, | ||
| { | ||
| name: "empty retain test", |
There was a problem hiding this comment.
| name: "empty retain test", | |
| name: "retain empty resources", |
| }, | ||
| }, | ||
| { | ||
| name: "normal retain test", |
There was a problem hiding this comment.
| name: "normal retain test", | |
| name: "retain non-empty resources", |
wy-lucky
force-pushed
the
main
branch
4 times, most recently
from
e112d66
to
cb118a7
Compare
| desiredContainer: map[string]interface{}{ | ||
| "name": "container-1", | ||
| "resources": map[string]interface{}{ | ||
| "cpu": "500m", |
There was a problem hiding this comment.
Should be requests or limits here?
There was a problem hiding this comment.
Yes, it has been updated.
wy-lucky
force-pushed
the
main
branch
2 times, most recently
from
14cb13f
to
f4fabc7
Compare
Some cloud vendors, when creating pods, add resource or dnsConfig configurations through webhooks, causing continuous update failures during deployment. Therefore, when distributing pods, we retain the spec.containers.resources and spec.dnsConfig field.