Add MultiClusterEngine to OCM package #427
Conversation
ddmitrie
requested review from
kononovn,
trewest,
achuzhoy and
cdvultur
as code owners
ddmitrie
force-pushed
the
multiclusterengine-pkg-addition
branch
3 times, most recently
from
a58b4ad to
cc71626
Compare
ddmitrie
force-pushed
the
multiclusterengine-pkg-addition
branch
3 times, most recently
from
7da4a0d to
080c630
Compare
|
@ddmitrie All new public functions/methods require unit tests. Can you add unit tests following these guidelines? https://github.com/openshift-kni/eco-goinfra?tab=readme-ov-file#note-every-new-package-requires-a-coverage-of-all-its-public-functions-with-unit-tests-unit-tests-are-located-in-the-same-package-as-the-resource-in-a-file-with-the-name-resource_testgo-examples-can-be-found-in-configmap_testgo-and-metallb_testgo |
| func NewMultiClusterEngineBuilder(apiClient *clients.Settings, name string) *MultiClusterEngineBuilder { | ||
| glog.V(100).Infof( | ||
| `Initializing new MultiClusterEngine structure with the following params: name: %s`, name) | ||
|
|
There was a problem hiding this comment.
Please check that apiClient is not nil before accessing it's members
| // PullMultiClusterEngine loads an existing MultiClusterEngine into MultiClusterEngineBuilder struct. | ||
| func PullMultiClusterEngine(apiClient *clients.Settings, name string) (*MultiClusterEngineBuilder, error) { | ||
| glog.V(100).Infof("Pulling existing MultiClusterEngine name: %s", name) | ||
|
|
There was a problem hiding this comment.
Please check that apiClient is not nil before accessing it's members
|
|
||
| glog.V(100).Infof("Getting MultiClusterEngine %s", builder.Definition.Name) | ||
|
|
||
| MultiClusterEngine := &mceV1.MultiClusterEngine{} |
There was a problem hiding this comment.
| MultiClusterEngine := &mceV1.MultiClusterEngine{} | |
| multiclusterengine := &mceV1.MultiClusterEngine{} |
| } | ||
|
|
||
| glog.V(100).Infof("Updating MultiClusterEngine %s", builder.Definition.Name) | ||
|
|
There was a problem hiding this comment.
Please check that the multiclusterengine exists before attempting to update
| glog.V(100).Infof("Updating MultiClusterEngine %s", builder.Definition.Name) | ||
|
|
||
| err := builder.apiClient.Update(context.TODO(), builder.Definition) | ||
| builder.Object = builder.Definition |
There was a problem hiding this comment.
This runs regardless of if the above operation is successful or not. if err != nil, you should return without updating builder.Object
ddmitrie
force-pushed
the
multiclusterengine-pkg-addition
branch
from
080c630 to
66a73cf
Compare
| glog.V(100).Infof("Deleting the MultiClusterEngine %s", builder.Definition.Name) | ||
|
|
||
| if !builder.Exists() { | ||
| return fmt.Errorf("multiclusterengine cannot be deleted because it does not exist") |
There was a problem hiding this comment.
if the object doesn't exist and Delete method is invoked probably worth returning success?
ddmitrie
force-pushed
the
multiclusterengine-pkg-addition
branch
from
66a73cf to
2a42ac7
Compare
|
Converted to Draft. Seems like there are some conflicts between this pkg and the assisted pkg. We need to implement these APIs ourselves to address them. |
No description provided.