populate default value in unmarshal #939
Conversation
|
@JiangtianLi , i saw OrchestratorType is using Then i can use OrchestratorProfile's UnmarshalJSON to do what we have now. We can eliminate a type definition. |
| return e | ||
| } | ||
| *m = MasterProfile(mm) | ||
| if m.Count == 0 { |
There was a problem hiding this comment.
If other fields in MasterProfile need to populate default value, it can be done inside this function.
rjtsdl
changed the title
| return e | ||
| } | ||
| *m = MasterProfile(mm) | ||
| if m.Count == 0 { |
There was a problem hiding this comment.
Does json.Unmarshal fill the empty field with 0?
There was a problem hiding this comment.
Yes, for int 0, for string ""
| // And do fields manipulation, such as populating default value | ||
| func (m *MasterProfile) UnmarshalJSON(b []byte) error { | ||
| // Need to have a alias type to avoid circular unmarshal | ||
| type aliasMasterProfile MasterProfile |
There was a problem hiding this comment.
otherwise, when you call MasterProfile.UnmarshalJSON, it causes a circular call to itself.
Because we have this, if mm is not the alias, it will be MasterProfile. Here is when circular call starts.
if e := json.Unmarshal(b, &mm); e != nil {
return e
}
|
LGTM. let me know when you are ready to checkin. |
|
There maybe concerns on why do we need to populate default values when we have My principle would be The populating in the unmarshal function is to fulfill the promise we made on the versioned api documents. Think about the scenario, in version 2018-09-30, we probably will switch default storageProfile to managedDisk. However, we won't change the existing versions default, which is to storageAccount. Make sense now? |
|
@weinong, thx for reviewing. I will do this trick for the other versions. And also, I am inclined to do this for 2017-07-01. I will have the PR ready by Monday. |
rjtsdl
force-pushed
the
jiren-unmarshalwithdefault
branch
from
9569e11 to
9384fc5
Compare
rjtsdl
changed the title
|
@acs-bot test this please |
|
@weinong , i get code ready. PTAL |
|
Reviewed 2 of 2 files at r2, 5 of 5 files at r3. Comments from Reviewable |
|
@rjtsdl LGTM. Yes, we can change orchestratorType in a separate PR. |
|
I will just merge it. Thx |
What this PR does / why we need it:
close api definition gap to public document
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #936Special notes for your reviewer:
One sample for masterCount, if looks good, I will apply to all the other fields.
Release note:
This change is