RDS Core: separate general methods from the sriov verification suite #65
Conversation
elenagerman
force-pushed
the
rds-adjustments
branch
from
564a1b7
to
7124b9b
Compare
|
@yprokule Hi! as was discussed, PTAL |
elenagerman
force-pushed
the
rds-adjustments
branch
from
7124b9b
to
150437f
Compare
| deploySa, err := deploySa.Create() | ||
|
|
||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Error creating SA %q in %q namespace: %v", | ||
| saName, nsName, err) | ||
| } |
There was a problem hiding this comment.
This should be inside Eventually block
|
|
||
| deploySa := serviceaccount.NewBuilder(rdscoreinittools.APIClient, saName, nsName) | ||
|
|
||
| if !deploySa.Exists() { |
There was a problem hiding this comment.
please add logging for a case that SA already exists, thanks
| err := deploySa.Delete() | ||
|
|
||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Error deleting ServiceAccount %q in %q namespace: %v", | ||
| saName, nsName, err) | ||
| } |
There was a problem hiding this comment.
This should be within Eventually block
| err := crbSa.Delete() | ||
|
|
||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Error deleting ClusterRoleBinding %q : %v", | ||
| rbacName, err) | ||
| } |
There was a problem hiding this comment.
ditto, should be within Eventually block
| crbSa, err := crbSa.Create() | ||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof( | ||
| "Error Creating ClusterRoleBinding %q : %v", crbSa.Definition.Name, err) | ||
| } |
There was a problem hiding this comment.
ditto, should be within Eventually block
| err := cmBuilder.Delete() | ||
|
|
||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Error deleting configMap %q : %v", | ||
| cmName, err) | ||
| } |
| cmResult, err := cmBuilder.Create() | ||
| if err != nil { | ||
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Error creating ConfigMap %q in %q namespace: %v", | ||
| cmName, nsName, err) | ||
| } | ||
|
|
| glog.V(rdscoreparams.RDSCoreLogLevel).Infof("Deleting deployment %q from %q namespace", | ||
| deploy.Definition.Name, nsName) | ||
|
|
||
| deploy, err = deploy.WithReplicas(int32(0)).Update() |
There was a problem hiding this comment.
no need to scale down deployment before deletion
|
@yprokule I'm sorry, but I do not understand why general methods like delete/create API objects cannot be outside the sriov validation test. What if we need it for the other operator's tests? For example, configmap, rbac, and sa config can be required for the keda config. |
@elenagerman what are U talking about? I didn't object about having some of the common methods in a dedicated file. What I said is that during your move of existing methods to a new file U broke the existing logic and were asked to fix it
U don't double check but create if it's missing which again affects existing logic. And what the problem to run this at the beginning of the test or in the setup block?
Again, not sure where U got the idea that we are returning to the original version - all U had to do was to address broken logic in the PR. Feel free to reopen PR and address comments or I might piggyback on this work and adjust it myself. Thank you! |
helper.go file created
verifySRIOVConnectivity was renamed to the verifyConnectivity and moved to the helper.go
createServiceAccount, deleteServiceAccount, deleteClusterRBAC, createClusterRBAC, deleteConfigMap, createConfigMap, deleteDeployments, findPodWithSelector, defineContainer, verifyMsgInPodLogs were moved to the helper.go
'if pState == "NonCompliant"' state was changed to the 'if pState == "NonCompliant"'. Depends on deployment type it can be some extra policies with no state at all be found and it can fail test with no reason