Adding Changes to add Accounttype only for specific models#167
Conversation
gravi21
requested review from
AravindhanBoopalan,
bhushan-mdn,
coolwater,
kelleyloder and
pt-nguyen
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
| log.WithFields(fields).Debugf("AddEstbFirmwareContext AcntId='%s' already present,fetching AccntPrds directly from ada", contextMap[common.ACCOUNT_ID]) | ||
| log.WithFields(fields).Debugf("AddEstbFirmwareContext AcntId='%s' received from grpsvc,now fetching AccntPrds", contextMap[common.ACCOUNT_ID]) | ||
|
|
||
| if Xc.AccountTypeModelSet.IsEmpty() || Xc.AccountTypeModelSet.Contains(strings.ToLower(contextMap[common.MODEL])) { |
There was a problem hiding this comment.
Hello @gravi21
I see the same AccountTypeModelSet.IsEmpty() || AccountTypeModelSet.Contains(...) condition being checked at both line 267 and line 277. Since contextMap[common.MODEL] doesn't change between these two points, the inner check at line 277 appears to be redundant.
There was a problem hiding this comment.
@RahulRengeshOfficial in line 277 case is for when we already have accountId in the request from device and in line 267 is for unknown accountId case.
All these is for workround until we get accounttype in /ada keyspace
|
|
||
| if accountType, ok := accountProducts["AccountType"]; ok && accountType != "" { | ||
| contextMap[common.ACCOUNT_TYPE] = accountType | ||
| if Xc.AccountTypeModelSet.IsEmpty() || Xc.AccountTypeModelSet.Contains(strings.ToLower(contextMap[common.MODEL])) { |
There was a problem hiding this comment.
Hello @gravi21
I see the same AccountTypeModelSet.IsEmpty() || AccountTypeModelSet.Contains(...) condition being checked before, Since contextMap[common.MODEL] doesn't change between these two points, the inner check appears to be redundant.
pt-nguyen
left a comment
pt-nguyen
left a comment
There was a problem hiding this comment.
Minor performance improvement to avoid calling IsValidMacAddress again.
| log.WithFields(fields).Debugf("AddEstbFirmwareContext AcntId='%s' received from grpsvc,now fetching AccntPrds", contextMap[common.ACCOUNT_ID]) | ||
|
|
||
| if Xc.AccountTypeModelSet.IsEmpty() || Xc.AccountTypeModelSet.Contains(strings.ToLower(contextMap[common.MODEL])) { | ||
| if util.IsValidMacAddress(contextMap[common.ESTB_MAC]) { |
There was a problem hiding this comment.
@gravi21 Can we cache the value of checking if mac address is valid so that we don't to do the same check again on line 285?
validMac := util.IsValidMacAddress(contextMap[common.ESTB_MAC])
if validMac {
...
| } | ||
|
|
||
| if xAccountId == nil && err != nil { | ||
| if util.IsValidMacAddress(contextMap[common.ECM_MAC_ADDRESS]) { |
There was a problem hiding this comment.
No need to call check again.
if validMac {
| log.WithFields(fields).Debugf("AddFeatureControlContext AcntId='%s' already present", contextMap[common.ACCOUNT_ID]) | ||
|
|
||
| if Xc.AccountTypeModelSet.IsEmpty() || Xc.AccountTypeModelSet.Contains(strings.ToLower(contextMap[common.MODEL])) { | ||
| if util.IsValidMacAddress(contextMap[common.ESTB_MAC]) { |
There was a problem hiding this comment.
Should cache value if mac is valid so don't have to call gain.
| log.WithFields(fields).Debugf("AddLogUploaderContext AcntId='%s' already present,fetching AccntPrds directly from ada", contextMap[common.ACCOUNT_ID]) | ||
|
|
||
| if Xc.AccountTypeModelSet.IsEmpty() || Xc.AccountTypeModelSet.Contains(strings.ToLower(contextMap[common.MODEL])) { | ||
| if util.IsValidMacAddress(contextMap[common.ESTB_MAC]) { |
There was a problem hiding this comment.
Should cache value if mac is valid so don't have to call gain.
6 participants
No description provided.