Remove nvidia.com/gpu.imex-domain label #1152
Conversation
| } | ||
| if imexDomainID == "" { | ||
| return nil, nil | ||
| return empty{}, nil |
There was a problem hiding this comment.
why empty{} instead of nil?
There was a problem hiding this comment.
In order to avoid explicit nil checks at the callsite, we have an empty{} Labeler which returns no labels.
There was a problem hiding this comment.
could we do the same for line (89/32)
There was a problem hiding this comment.
could we do the same for line (89/32)
No. On that line we're returning an actual error and we want to return nil as the labeler.
elezar
requested review from
klueska,
tariq1890,
ArangoGutierrez and
cdesiniotis
|
@tariq1890 we should look into rolling back the changes introduced in NVIDIA/gpu-operator@5525636 and NVIDIA/gpu-operator#1070 as they are no longer needed. |
| } | ||
| if imexDomainID == "" { | ||
| return nil, nil | ||
| return empty{}, nil |
There was a problem hiding this comment.
could we do the same for line (89/32)
| return nil, fmt.Errorf("failed to open imex config file: %v", err) | ||
| } | ||
| defer imexConfigFile.Close() | ||
|
|
||
| clusterUUID, cliqueID, err := getFabricIDs(devices) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
| return nil, err | |
| return empty{}, nil |
There was a problem hiding this comment.
No. Here we are returning an actual error. Do you have a motivation as to why we want to ignore it?
This removes the logic for constructing an nvidia.com/gpu.imex-domain label. This also means that the logic to include the imex nodes_config.cfg file in the GFD container is not required. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
remove-imex-domain-label
branch
from
17afec9 to
dd78f3d
Compare
elezar
dismissed
ArangoGutierrez’s stale review
The proposed changes are not valid since we are actually returning an error in the other instance.
This removes the logic for constructing an nvidia.com/gpu.imex-domain label. This also means that the logic to include the imex nodes_config.cfg file in the GFD container is not required.
This is a backport of #1147