Check restoration hash in NetBox before updating resource in NetBox #266
Conversation
There was a problem hiding this comment.
Looks good to me, would be great if @alexandernorth could also spend a few minutes to review the code.
A few minor things:
- "mismatch" is misspelled. search/replace "issmatch" with "ismatch"
- "restoration has" should be "restoration hash"
| if err != nil { | ||
| updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, | ||
| corev1.EventTypeWarning, err.Error()) | ||
| return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ |
There was a problem hiding this comment.
This is missing a check if updateStatusErr != nil
There was a problem hiding this comment.
Added, same for the other locations. I also added some return statements that were missing.
| } | ||
| if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, | ||
| corev1.EventTypeWarning, o.Spec.IpAddress); updateStatusErr != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ |
There was a problem hiding this comment.
This is missing a check if updateStatusErr != nil
| if err != nil { | ||
| updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, | ||
| corev1.EventTypeWarning, err.Error()) | ||
| return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ |
There was a problem hiding this comment.
This is missing a check if updateStatusErr != nil
| } | ||
| if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, | ||
| corev1.EventTypeWarning, prefix.Spec.Prefix); updateStatusErr != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ |
There was a problem hiding this comment.
This is missing a check if updateStatusErr != nil
pkg/netbox/api/ip_address_test.go
Outdated
| Ipam: mockIPAddress, | ||
| } | ||
|
|
||
| ipAddressModel := ipAddressModel("fioaf9289rjfhaeuih") |
There was a problem hiding this comment.
Should this be declared as a const - it appears elsewhere too
There was a problem hiding this comment.
I've now defined variable for all hashes, so it's also easier to read.
pkg/netbox/api/ip_range_test.go
Outdated
| StartAddress: &startAddress, | ||
| EndAddress: &endAddress, | ||
| CustomFields: map[string]interface{}{ | ||
| config.GetOperatorConfig().NetboxRestorationHashFieldName: "different hash", |
There was a problem hiding this comment.
Other controller uses something that looks like a hash - this could be aligned to either be text description or a hash for all controllers?
There was a problem hiding this comment.
Agree, I chose to use sample hashes in all the tests.
Thanks, the typos are fixed now. |
bruelea
force-pushed
the
fix/check-restoration-hash-in-case-lock-times-out
branch
from
956b8c4 to
d352a66
Compare
In case the lock times out before a resource claimed by a prefix was successfully created in NetBox, two claims can get the same value assigned. By checking the restoration hash a resource has in NetBox assignments of the same resource to different claims can be avoided.