Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jwa(front): Fix the workspace volume form's inputs #7029

Conversation

elenzio9
Copy link
Contributor

@elenzio9 elenzio9 commented Mar 6, 2023

The PR ensures that the intermediate FormControls for the name, size and access modes will be getting updated correctly when the frontend gets the ConfigMap data. It also adds some integration tests for each field to ensure that the form will have the correct values once it gets the ConfigMap.

Related issue: #6741

@elenzio9 elenzio9 changed the title jwa(front): Fix the workspace volume form's inputs WIP - jwa(front): Fix the workspace volume form's inputs Mar 6, 2023
@elenzio9 elenzio9 force-pushed the feature-elena-jwa-workspace-volume-form-bug branch from 7885f7f to 102c216 Compare March 10, 2023 16:51
@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Mar 10, 2023
@elenzio9 elenzio9 changed the title WIP - jwa(front): Fix the workspace volume form's inputs jwa(front): Fix the workspace volume form's inputs Mar 10, 2023
@elenzio9 elenzio9 force-pushed the feature-elena-jwa-workspace-volume-form-bug branch from 102c216 to 62fafa8 Compare March 10, 2023 17:49
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this bug @elenzio9! The feature seems to work as expected. Here are some minor comments on the tests.

Comment on lines 86 to 87
//
expect(nameValue).equal('-workspace'); // '{notebook-name}-workspace' is the name value of the config fixture
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here was probably to move the comment from line 87 to where the // are now? Nit but I think it 'd be better this way in order to avoid having a really long line.

Comment on lines 66 to 67
cy.get('[data-cy="workspace volume"]')
.find('[data-cy="volume name input"]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the cy.get().find() chain is redundant here and you could convert this to cy.get('[data-cy="volume name input"]').

The same goes for all cy.get().find() in the tests introduced. There are cases where we might need to chain cy.get() with .find() but as far as I 'm concerned, this is not the case here. WDYT @elenzio9 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orfeas-k Thanks for the comments! Just pushed a fixup commit. Please take a look!

The size form input was not showing the correct value, once the frontend
got the ConfigMap defaults. We should be updating the intermediate FormControl
when the data from the ConfigMap arrives at the frontend.

Signed-off-by: Elena Zioga <elena@arrikto.com>
The access mode form input was not showing the correct value, once the frontend
got the ConfigMap defaults. We should be updating the intermediate FormControl
when the data from the ConfigMap arrives at the frontend.

Signed-off-by: Elena Zioga <elena@arrikto.com>
The name form input was not showing the correct value, once the frontend
got the ConfigMap defaults. We should be updating the intermediate FormControl
when the data from the ConfigMap arrives at the frontend.

Signed-off-by: Elena Zioga <elena@arrikto.com>
All new volumes will have a default value of 5Gi. This includes the 'Add
new volume' button for both the workspace and data volumes.

Signed-off-by: Elena Zioga <elena@arrikto.com>
@elenzio9 elenzio9 force-pushed the feature-elena-jwa-workspace-volume-form-bug branch from 62fafa8 to ea6e7d0 Compare March 22, 2023 12:56
Add integration tests with Cypress to ensure that the form will have the
correct values once it gets the ConfigMap.

Signed-off-by: Elena Zioga <elena@arrikto.com>
@elenzio9 elenzio9 force-pushed the feature-elena-jwa-workspace-volume-form-bug branch from ea6e7d0 to dc04644 Compare March 22, 2023 13:10
@orfeas-k
Copy link
Contributor

It looks good, I 'll go ahead and merge! @elenzio9
/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: orfeas-k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 228d21d into kubeflow:master Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants