Fix AdminAPI representation of PreconfiguredNSG status #3933
Conversation
tsatam
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
kimorris27,
bitoku and
fahlmant
as code owners
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
tsatam
dismissed stale reviews from jhoreman, s-fairchild, and bennerv
via
b15a5b3
|
Please rebase pull request. |
… doc to adminapi representation
… PreconfiguredNSG
tsatam
force-pushed
the
tsatam/hotfix-fix-preconfigurednsg-adminapi
branch
from
b15a5b3
to
90f57cc
Compare
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| @@ -179,6 +179,7 @@ func (c openShiftClusterConverter) ToExternal(oc *api.OpenShiftCluster) interfac | |||
| } | |||
|
|
|||
| if oc.Identity != nil { | |||
| out.Identity = &ManagedServiceIdentity{} | |||
There was a problem hiding this comment.
You should check if out.Identity == nil or this will clobber anything that is already there. This would have been a 100% panic if out.Identity were always nil. Is it?
There was a problem hiding this comment.
The out object is created from scratch earlier up in this function (L18-75), so we don't need to worry about clobbering any existing Identity properties, out.Identity is always nil at this point.
There was a problem hiding this comment.
So are you saying this code would always panic? Your original comment made it seem like it was normally fine and only having a panic in MIWI
There was a problem hiding this comment.
The if statement this line is contained in, if oc.Identity != nil, would only be true for MIWI clusters, but the code within that if block would always panic since out.Identity was always nil.
* Fix spelling of preconfiguredNSG JSON tag * Copy PreconfiguredNSG property during internal->admin conversion * Fix nil pointer dereference when converting workload identity cluster doc to adminapi representation * Update admin openshiftcluster_putorpatch tests to expect property for PreconfiguredNSG
* Fix spelling of preconfiguredNSG JSON tag * Copy PreconfiguredNSG property during internal->admin conversion * Fix nil pointer dereference when converting workload identity cluster doc to adminapi representation * Update admin openshiftcluster_putorpatch tests to expect property for PreconfiguredNSG
Which issue this PR addresses:
Fixes ARO-11975
What this PR does / why we need it:
Ensures that the status of the cluster document's
networkProfile.preconfiguredNSGproperty is included on Admin API representations of the document.Test plan for issue:
Is there any documentation that needs to be updated for this PR?
No
How do you know this will function as expected in production?
Testing done above