Closed skurnevich closed 3 months ago
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?
@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 - https://github.com/zowe/zen/pull/244#issuecomment-2312597551 , and I'll proceed with a quick round of testing before final approval.
Thanks
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
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:
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.