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

Remove ConfigService.ts, Enahance Certifcate Stage #114

Merged
merged 68 commits into from
Mar 8, 2024

Conversation

timgerstel
Copy link
Contributor

@timgerstel timgerstel commented Jan 30, 2024

Proposed changes

This PR combines these two 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

timgerstel and others added 30 commits October 26, 2023 11:58
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…efactor and implement form change handler

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…unning zwe init certificates

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…ly and remove console logs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…recate ConfigurationStore somehow

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…ertificateStage

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
timgerstel and others added 18 commits January 30, 2024 02:01
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…to cfgRefactorAndVerifyCerts

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
…Certs

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
@@ -64,8 +64,11 @@ contextBridge.exposeInMainWorld('electron', {
checkDirOrCreate(connectionArgs: IIpcConnectionArgs, location: string) {
return ipcRenderer.invoke("check-dir-or-create", connectionArgs, location);
},
installButtonOnClick(connectionArgs: IIpcConnectionArgs, installationArgs: {installDir: string, javaHome: string, nodeHome: string, installationType: string, userUploadedPaxPath: string, smpeDir: string}, version: string, zoweConfig: any) {
return ipcRenderer.invoke("install-mvs", connectionArgs, installationArgs, version, zoweConfig);
installButtonOnClick(connectionArgs: IIpcConnectionArgs, installationArgs: {installDir: string, javaHome: string, nodeHome: string, installationType: string, userUploadedPaxPath: string, smpeDir: string}, version: string, zoweConfig: any, skipDownload: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

what is this skipDownload stuff for? If it's for smpe, why don't we just check if smpeDir exists? Wasn't that the reason why we made that variable in the first place instead of setting installationDir = smpeDir and deleting smpeDir entirely

Also, it looks like it's never actually set any where, except 1 default
window.electron.ipcRenderer.installButtonOnClick(connectionArgs, installationArgs, version, yaml, skipDownload ?? false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was for QA testing, not a prod solution. It was so @struga0258 can skip the pax download process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Yes, we need a better solution for the smpe step

Copy link
Member

@DivergentEuropeans DivergentEuropeans left a comment

Choose a reason for hiding this comment

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

we need a better solution for the smpe step/QA testing hack

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
@@ -35,11 +35,16 @@ export class InstallActions {
}
}

runZweInitCertificates(connectionArgs: IIpcConnectionArgs,
installationArgs: {installationDir: string}, zoweConfig: any){
return new FTPInstallation().initCertificates(connectionArgs, installationArgs.installationDir, zoweConfig)
Copy link
Member

@DivergentEuropeans DivergentEuropeans Mar 6, 2024

Choose a reason for hiding this comment

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

This doesn't seem right. It seems like

  1. initCertificates( ) should should belong in Installation class, with initSecurity & the others +
    initCertificates should be made public
  2. change this to
    return this.strategy.initCertificates(connectionArgs, installationArgs.installationDir, zoweConfig);

Copy link
Member

@DivergentEuropeans DivergentEuropeans Mar 6, 2024

Choose a reason for hiding this comment

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

Also, the commands below don't include the Zwe part from method name so I guess we could remove that for organization's sake

return {status: false, details: `Error unpaxing Zowe archive: ${unpax.details}`};
}
} else {
download = upload = unpax = {status: true, details : ''}
Copy link
Member

Choose a reason for hiding this comment

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

Never seen that b4 for some reason, cool 👍

return ProgressStore.set('certificate.uploadYaml', false);;
}
ProgressStore.set('certificate.uploadYaml', uploadYaml.status);
const script = `cd ${installDir}/runtime/bin;\n./zwe init certificate --update-config -c ${installDir}/zowe.yaml`;
Copy link
Member

Choose a reason for hiding this comment

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

you no longer need "\n" as it's handled automatically however we do want to make sure we're using "&&" instead of ";"

return res;
});

ipcMain.handle('zwe-init-certificates', async (event, connectionArgs, installationArgs, zoweConfig) => {
Copy link
Member

Choose a reason for hiding this comment

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

why does this have zwe-init-certificates but below is init-apf ?

…allation class

Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Signed-off-by: Timothy Gerstel <tim.gerstel@gmail.com>
Copy link
Member

@DivergentEuropeans DivergentEuropeans left a comment

Choose a reason for hiding this comment

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

Changes addressed

@DivergentEuropeans DivergentEuropeans merged commit ba09467 into v2.x/staging Mar 8, 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