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

Chore/planning stage #244

Merged
merged 5 commits into from
Aug 29, 2024
Merged

Chore/planning stage #244

merged 5 commits into from
Aug 29, 2024

Conversation

skurnevich
Copy link
Collaborator

  • Get rid of unused vars and multiple logical duplicates like setPlanningStatus, setPlanningState, setNextStepEnabled etc
    All of them were just adding useless lines of code. setNextStepEnabled and setPlanningStatus
    Are controlled by setPlanningState which is dependent on jobHeaderSaved and locationsValidated. So these two last variables are controlling everything, there is no need to explicitly set any of above.

  • Removed RBAC, Cookie ID, Job ID, job prefix as not relevant to installation, we will add it to the misc config at the end of installation.

  • Remove Editor related code, it was not used as of now but making a mess, I agree that it may be used for displaying large errors, but we can get add it back when implementing a feature.

  • Fixed some storage duplications
    Example of duplication: setLocationsValidated() sets component state and uses it for rendering, dispatch(setIsLocationValid()) sets redux state but not uses it, instead, it obscurely sets it to the localStorage via setPlanningStageStatus for persistence
    This can be fixed by having a selector for isLocationValid in the planning slice and using it instead of component state.
    And it is getting worse when there are too many of these, like in this piece of code:

        const formChangeHandler = (key?: string, value?: (string | number), installationArg?: string) => {
          setIsLocationsUpdated(true);
          setPlanningStatus(false);
          setLocationsValidated(false);
          dispatch(setPlanningStatus(false));
          dispatch(setNextStepEnabled(false));

There is a typo in setPlanningStatus, it should be setPlanningState, but as it is too similar it was never spotted and component state was not updated here, I guess, this lead to adding more explicit setters everywhere, like a snowball.

I didn’t fix all the storage issues as there are more files affected. Like the JobStatement is still stored twice, in electron connection store and in the localStorage StageProgressStatus, that is potential source of issues. And there is still some arguable moment in classification, like what store/slice the job header should belong. Also we still need to remove directories and node/java from installationArgs as it is now stored in yaml. I’ll try to cover this in the next PR with overall storage review.

  • Fixed java and home config setting if found in JAVA_HOME and NODE_HOME

  • Added auto config update with z/osmf host = connection host as it is true for most of the use cases.

  • Rolled back space check, made it non-interrupting, so it shows the notification but allows to continue

  • Removed step 2 as useless on that stage, added green marks for validated fields.

  • Probably there are more fixes that I can’t recall now, and It is still far from perfect but I’ve tried to limit the changes in this PR by modifying just one file to make it easier to review and to avoid merge conflicts.

Signed-off-by: Sergei Kurnevich <[email protected]>
Signed-off-by: Sergei Kurnevich <[email protected]>
@sakshibobade21
Copy link
Contributor

sakshibobade21 commented Aug 27, 2024

Since the user cannot toggle the z/OSMF attributes checkbox, can we remove both the checkbox and the line that says "Set z/OSMF Attributes (Recommended)" entirely?

Also, don't we need Job ID and Job prefix to retrieve information about the job status, logs, and output?

@sakshibobade21
Copy link
Contributor

@skurnevich, thank you for submitting the PR.

The changes look excellent. You effectively handled the major code cleanup, both related and unrelated to maintaining the "state" of the planning stage.
The removal of redundant code, unused state variables, and unnecessary checks, along with the elimination of additional steps through the node and Java version checks, is well implemented.

The PR also have the quick validation of theUSS paths and prevents users from disabling z/OSMF.

Overall, this is a great improvement. I have only one minor comment here - #244 (comment) , and I'll proceed with a quick round of testing before final approval.

Thanks

…ions, removed zosmf checkbox

Signed-off-by: Sergei Kurnevich <[email protected]>
@skurnevich
Copy link
Collaborator Author

Since the user cannot toggle the z/OSMF attributes checkbox, can we remove both the checkbox and the line that says "Set z/OSMF Attributes (Recommended)" entirely?

Agree, i've removed the z/OSMF checkbox as redundant for now, also probably the entire z/OSMF section could be moved later to the certificates section as, i believe, it is not being used prior to that.

Also, don't we need Job ID and Job prefix to retrieve information about the job status, logs, and output?

These are not used by ZEN, it is for Zowe started task and some rules configuration, when ZEN submits jobs it uses Job ID returned directly by zos-node-accessor

Copy link
Contributor

@sakshibobade21 sakshibobade21 left a comment

Choose a reason for hiding this comment

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

The PR looks great - #244 (comment)

@skurnevich skurnevich merged commit 250f248 into v2.x/staging Aug 29, 2024
4 of 5 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