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

Dataset Validation On Installation Stage #233

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

sakshibobade21
Copy link
Contributor

Proposed changes

This PR addresses Issue: This PR adds the dataset validations on the installation stage

This PR depends upon the following PRs:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Change in a documentation
  • Refactor the code
  • Chore, repository cleanup, updates the dependencies.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

  • If the changes in this PR are meant for the next release / mainline, this PR targets a "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • video or image is included if visual changes are made
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

Further comments

for (const key of Object.keys(setupYaml)) {
let value = setupYaml[key];

if (typeof value === 'object' && value !== null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this part is hardcoded to the current state of zowe.yaml, assuming that there is just one value in the parmlibMembers object - {zis: ZWESIP00}.
If we add one more value there later it would not be validated, so instead we should recursively go to validateDatasets with that new object or flatten data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates this. Thank you.

return false;
}
}
if(Object.keys(setupYaml).length < 8) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, now we have 8 values there but it can easily change, moreover for instance zowe.setup.dataset.authLoadlib is actually optional.
This better to be controlled by schema, but i can't see it for zowe.setup.dataset, so i guess for now we can remove hardcoded numbers and just validate it in the UI by not allowing empty values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed these changes. Thank you.

<JsonForm schema={setupSchema} onChange={handleFormChange} formData={setupYaml}/>
{!isFormValid && formError &&
<div style={{ display: 'flex', marginBottom: '10px' }}>
<ErrorIcon color="error" sx={{ fontSize: 20 }}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we have some unified errors popup in the header? why we have different approach here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially considered displaying the form errors right below the form after clicking "Install MVS Datasets." However, let's stick to using the unified error messages in the header instead.

Update this. Thank you.

Copy link
Collaborator

@skurnevich skurnevich left a comment

Choose a reason for hiding this comment

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

LGTM, however, in future i would prefer it to be validated via schema like https://github.com/zowe/zowe-install-packaging/blob/v2.x/staging/schemas/server-common.json#L39
But we need to update schema for that

@skurnevich skurnevich merged commit b831061 into v2.x/staging Aug 8, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants