Send filesystem type to Wanda #2126
Conversation
dottorblaster
force-pushed
the
fs-type-ascs
branch
from
a7609d0 to
40f84c1
Compare
dottorblaster
force-pushed
the
fs-type-ascs
branch
2 times, most recently
from
3be197a to
da2866f
Compare
dottorblaster
force-pushed
the
fs-type-ascs
branch
from
6d1c0e2 to
be21d55
Compare
| @@ -43,11 +47,12 @@ export function ClusterDetailsPage() { | |||
| target_type: TARGET_CLUSTER, | |||
| cluster_type: type, | |||
| ensa_version: ensaVersion, | |||
| filesystem_type: filesystemType, | |||
There was a problem hiding this comment.
ensa_version and filesystem_type are only relevant when clusterType is ascs_ers.
Here we are sending everything regardless of the context (ie ensa version/fs type sent along on a hana cluster).
It is true that some information would be ignored during query because they do not apply to a cluster type (ie ensa version very likely won't have any effect on hana checks), but I personally find it a bit confusing, inconsistent and too implicit.
We usually tend to prefer explicitness over implicitness.
| @@ -34,6 +35,9 @@ export function ClusterDetailsPage() { | |||
| const lastExecution = useSelector(getLastExecution(clusterID)); | |||
|
|
|||
| const ensaVersion = useSelector((state) => getEnsaVersion(state, clusterID)); | |||
| const filesystemType = useSelector((state) => | |||
There was a problem hiding this comment.
We do know from our domain that a filesystemType only makes sense in the context of an ascs/ers cluster and that there is no such thing like filesystemType to be considered in a non ascs/ers context.
However in all non ascs/ers scenarios (ie hana) filesystemType would end up being mixed_fs_types.
The problem, imho, is not that a variable gets a value, but that this way we're modeling something that has no correspondence in our domain. I believe we are not properly considering cluster type here.
I am aware that at the end whatever value filesystemType gets it would just be sent down to the catalog api and eventually, in the correct scenario, very likely, ignored by wanda. But as mentioned in the other comment I am not a fan of that either.
|
Another note: we are not sending the filesystem type (nor ensa version) to wanda during execution. Only in catalog loading |
|
@nelsonkopliku thanks for all the comments, I'll follow up to them once we have ENSA version identification properly done and merged 👍 |
dottorblaster
force-pushed
the
fs-type-ascs
branch
2 times, most recently
from
d9e20bc to
ceacd69
Compare
dottorblaster
force-pushed
the
fs-type-ascs
branch
from
ceacd69 to
8099176
Compare
|
@nelsonkopliku removed the leftover test, thanks. I'd like to address the differentiation between ascs/ers and other cluster types in the frontend in a subsequent PR, what do you think? |
Thank you! Yes, it makes sense to me as well following that up in another PR 👍 Still finishing the review tho 😄 |
There was a problem hiding this comment.
Thanks for addressing sending the env to wanda.
Besides some minor things my main feedback is about AscsErsClusterExecutionEnv. I believe we can avoid that and rely on the cluster type of the already existent ClusterExecutionEnv by doing some pattern matching.
dottorblaster
force-pushed
the
fs-type-ascs
branch
from
82eeede to
c8d4718
Compare
Description
Sending filesystem type to Wanda when selecting checks for a cluster, and during an execution request.
How was this tested?
Jest tests added, Elixir tests added