dashboard icon indicating copy to clipboard operation
dashboard copied to clipboard

add `data directory` configuration component to advanced tab in rke2 create/edit cluster interface

Open aalves08 opened this issue 1 year ago • 5 comments

Summary

Fixes #10824

Occurred changes and/or fixed issues

  • add data directory configuration component to advanced tab in rke2 create/edit cluster interface
  • add unit test for data directory configuration component

Technical notes summary

  • full manual test is dependent on https://github.com/rancher/rancher/issues/45038

Areas or cases that should be tested

  • Go to RKE2 cluster creation interface and make sure the data configuration area is present on the Advanced tab of Cluster Configuration
  • Make sure data persists and all works fine
  • Make sure that cluster editing, the data configuration is disabled (not allowed to change the values)

Areas which could experience regressions

Screenshot/Video

NOTE: this video still doesn't cover the edit scenario properly, since we still don't have the changes needed on the backend to persist the data, but it proves the payload sent is correct

https://github.com/rancher/dashboard/assets/97888974/2f09bc61-8c02-4927-8ebf-98257261fc6d

Checklist

  • [x] The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • [x] The PR has a Milestone
  • [x] The PR template has been filled out
  • [x] The PR has been self reviewed
  • [x] The PR has a reviewer assigned
  • [x] The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • [x] The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

aalves08 avatar May 16 '24 12:05 aalves08

I think even though you are not allowed to change the values later, you should still be able to see the exact values you entered when you created the cluster. For example, I added separate paths for each of the three inputs when I created the cluster but this is what I see in edit mode: image

momesgin avatar May 16 '24 18:05 momesgin

I think even though you are not allowed to change the values later, you should still be able to see the exact values you entered when you created the cluster. For example, I added separate paths for each of the three inputs when I created the cluster but this is what I see in edit mode: image

That's because the backend part is still not done. The data doesn't persist yet. It's normal, for now 😬

But they payload is sent correctly. Check the RFC doc linked to the SURE issue where they show the agreed data struct.

aalves08 avatar May 16 '24 19:05 aalves08

I think even though you are not allowed to change the values later, you should still be able to see the exact values you entered when you created the cluster. For example, I added separate paths for each of the three inputs when I created the cluster but this is what I see in edit mode: image

That's because the backend part is still not done. The data doesn't persist yet. It's normal, for now 😬

But they payload is sent correctly. Check the RFC doc linked to the SURE issue where they show the agreed data struct.

now that makes sense! 😅

momesgin avatar May 16 '24 21:05 momesgin

I removed the optional chaining before (I agree it should not be needed) but this caused failures in at least another unit test... I just re-added them to move this forward :P

aalves08 avatar May 17 '24 08:05 aalves08

I removed the optional chaining before (I agree it should not be needed) but this caused failures in at least another unit test... I just re-added them to move this forward :P

It seems you can fix all those failing tests by adding:

dataDirectories: {
   systemAgent:  '',
   provisioning: '',
   k8sDistro:    '',
}

to: https://github.com/rancher/dashboard/blob/b42c20ed5640d91b20a1f100e79c99a2522c4a50/shell/edit/provisioning.cattle.io.cluster/tests/utils/cluster.ts#L62

momesgin avatar May 17 '24 15:05 momesgin

I removed the optional chaining before (I agree it should not be needed) but this caused failures in at least another unit test... I just re-added them to move this forward :P

It seems you can fix all those failing tests by adding:

dataDirectories: {
   systemAgent:  '',
   provisioning: '',
   k8sDistro:    '',
}

to:

https://github.com/rancher/dashboard/blob/b42c20ed5640d91b20a1f100e79c99a2522c4a50/shell/edit/provisioning.cattle.io.cluster/tests/utils/cluster.ts#L62

@momesgin code updated 🙏

aalves08 avatar May 20 '24 13:05 aalves08

I am getting "[Vue warn]: Invalid prop: type check failed for prop "value". Expected Object, got Undefined

found in

---> <DirectoryConfig> at shell/edit/provisioning.cattle.io.cluster/tabs/DirectoryConfig.vue <Advanced> at shell/edit/provisioning.cattle.io.cluster/tabs/Advanced.vue <Tab> at shell/components/Tabbed/Tab.vue <Tabbed> at shell/components/Tabbed/index.vue <CruResource> at shell/components/CruResource.vue <CreateEditView> at shell/edit/provisioning.cattle.io.cluster/rke2.vue <CruResource> at shell/components/CruResource.vue <CruCluster> at shell/edit/provisioning.cattle.io.cluster/index.vue <CreateEditView> at shell/components/ResourceDetail/index.vue <ClusterResourceCreate> at shell/pages/c/_cluster/_product/_resource/create.vue <Shell/components/templates/default.vue> at shell/components/templates/default.vue <Root> error thrown if I am trying to save rke2 cluster with default configuration.

https://github.com/rancher/dashboard/assets/133266076/d885acc6-7484-44a0-ac3a-3c190e4f73af

eva-vashkevich avatar May 24 '24 21:05 eva-vashkevich

Gm @eva-vashkevich . This was dependent on a backend issue https://github.com/rancher/rancher/issues/45038 but all of the work has been merged, afaik. I am going to give this a test with a new backend image. Since I've done the frontend part without the backend being finished, I totally forgot to test it manually and merged this (all based on the RFC doc). 🙏

aalves08 avatar May 27 '24 08:05 aalves08

Looking at the associated PRs, I don't think the backend part is completed to this date.... 🤔 The console message is not pretty, but I don't think there's a high risk with the current work merged. It allows for creating/edit of a cluster + the console message doesn't appear unless you're running the UI locally.

I've just pinged the backend team to get more information about the current status of their work and it's expected to be completed.

aalves08 avatar May 27 '24 09:05 aalves08

@aalves08 Thank you for looking into it

eva-vashkevich avatar May 27 '24 15:05 eva-vashkevich