[FEAT]: Ceph Config API #24
Conversation
| } | ||
| return iService > jService | ||
| default: | ||
| return false |
There was a problem hiding this comment.
what is default case here? should we skip sorting in this case?
| } | ||
| serviceStr, found := serviceTypeMap[*service] | ||
| if !found { | ||
| serviceStr = service.String() |
There was a problem hiding this comment.
if i got it right, this line means that we don't have suitable enum for given value. should it be error? or at least log error?
There was a problem hiding this comment.
This is more of a fallback. If we do not have a suitable enum value, a test will fail.
pkg/cephconfig/config.go
Outdated
| return nil, fmt.Errorf("failed to unmarshal config index JSON: %w", err) | ||
| } | ||
|
|
||
| // Check if jsonArray is sorted by Name |
There was a problem hiding this comment.
don't need to check. since json is embedded and hadrcoded, we can check sort order in unit test
| sortedCluster []string, | ||
| fetchNew func(name string) (ConfigParamInfo, error), | ||
| ) ([]ConfigParamInfo, error) { | ||
| result := make([]ConfigParamInfo, 0, len(sortedCluster)) |
There was a problem hiding this comment.
should capacity be a max(sortedBase,sortedCluster)?
There was a problem hiding this comment.
this should be okay because we will not have more than len(sortedCluster) params (the number of params allowed in the current cluster)
| i++ | ||
| } | ||
| } | ||
| // Any remaining names in cluster are new params |
There was a problem hiding this comment.
should we also handle case where i < len(sortedBase)?
There was a problem hiding this comment.
if i < len(sortedBase) after the loop, it means we skipped adding those elements from sortedBase (from the base JSON file) that were not in the current cluster. This is what we want, so no need to handle separately.
Closes #21