(feat): Adding dynamic client & deploymentconfig object support #199
Conversation
Signed-off-by: Raj <raj.das@mayadata.io>
Signed-off-by: Raj <raj.das@mayadata.io>
imrajdas
requested review from
chandankumar4,
ksatchit and
rahulchheda
as code owners
imrajdas
force-pushed
the
deploymentconfig
branch
2 times, most recently
from
7b00ec7 to
c5209e1
Compare
imrajdas
force-pushed
the
deploymentconfig
branch
4 times, most recently
from
5acb04a to
1e7d2c8
Compare
Signed-off-by: Raj <raj.das@mayadata.io>
| ) | ||
|
|
||
| var ( | ||
| gvr = schema.GroupVersionResource{ |
There was a problem hiding this comment.
Can you name this a little Better? deploymentConfigGVR
There was a problem hiding this comment.
It was there, @chandankumar4 told to change it to gvr
There was a problem hiding this comment.
gvr is also good, As this variable is already inside the deploymentconfig file
There was a problem hiding this comment.
gvr is too generic, still would be better to change it something suitable.
There was a problem hiding this comment.
gvr is GroupVersionResource still the variable doesn't explain for which resource GVR we have.
There was a problem hiding this comment.
Then we can name it as deploymentConfigGVR earlier it was dcGVR
| } | ||
| ) | ||
| // CheckDeploymentAnnotation will check the annotation of deployment | ||
| func CheckDeploymentConfigAnnotation(clientSet dynamic.Interface, engine *chaosTypes.EngineInfo) (*chaosTypes.EngineInfo, error) { |
There was a problem hiding this comment.
Can we use this variable dynamicClient instead to clientSet
There was a problem hiding this comment.
In future we will migrating to dynamic-client, its good to have this name
There was a problem hiding this comment.
Still we will use dynamicClient and remove clientSet
There was a problem hiding this comment.
dynamicClient set is very different from normal clientSet thats why it would be better to let anyone going through the code know that this function is using dynamicClient
|
|
||
| // This will check and count the total chaos enabled application | ||
| func checkForChaosEnabledDeploymentConfig(deploymentConfigList *unstructured.UnstructuredList, engine *chaosTypes.EngineInfo) (*chaosTypes.EngineInfo, int, error) { | ||
|
|
There was a problem hiding this comment.
Can we validate that the GVR of *unstructured.UnstructuredList matches with deploymentConfig GVR, and then start to validate.
There was a problem hiding this comment.
Or add the validation in its above function
| // This will check and count the total chaos enabled application | ||
| func checkForChaosEnabledDeploymentConfig(deploymentConfigList *unstructured.UnstructuredList, engine *chaosTypes.EngineInfo) (*chaosTypes.EngineInfo, int, error) { | ||
|
|
||
| chaosEnabledDeploymentConfig := 0 |
There was a problem hiding this comment.
Add a nil check for *unstructured.UnstructuredList before iterating into items
There was a problem hiding this comment.
Will raised another Pr for this
…uschaos#199) * Adding deploymentconfig annotation check support Signed-off-by: Raj <raj.das@mayadata.io>
Signed-off-by: Raj raj.das@mayadata.io
Issue- litmuschaos/litmus#1406