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

zwe install implementation details #4044

Open
Martin-Zeithaml opened this issue Oct 9, 2024 · 6 comments
Open

zwe install implementation details #4044

Martin-Zeithaml opened this issue Oct 9, 2024 · 6 comments
Labels
clarification Config Manager Related to the config manager component zwe

Comments

@Martin-Zeithaml
Copy link
Contributor

Martin-Zeithaml commented Oct 9, 2024

As moving to JCL templates, zwe install should be rewritten too. All we need for installation is zowe.runtimeDirectory and zowe.setup.dataset.prefix.

Current implementation supports 2 ways:

  • zwe install --config /path/to/zowe.yaml
  • zwe install --ds-prefix ZOWE.V3R0M0
    • No need for zowe.yaml, this is handy for quick installation, developers...

There is a problem with import, if we use the same approach as for other commands:

bin/command/install/index.sh
+-->  "configmgr cli.js"

bin/command/install/cli.js
+--> "index.execute()"

bin/command/install/index.js
+--> "install implementation"

At some place we need to use import * as config or import * as configmgr: this will cause the code will require config. But we need also code path for --ds-prefix, which means without config.

Solutions:

Update

Problems:

  • With --ds-prefix user does not have an option to customize JCL header - unless we will add another parameter
  • JCL ZWEINSTL creates datasets and copies files (SHell in BPXBATCH)
    • At this moment, it will always fail for the second submit (ALLOCATE will fail)
    • When --allow-overwrite is used, we need to skip allocating or delete datasets or have 2 JCL, one with allocate+shell and the second with shell only or something better
@Martin-Zeithaml Martin-Zeithaml added clarification zwe Config Manager Related to the config manager component labels Oct 9, 2024
@1000TurquoisePogs
Copy link
Member

I think the 2 CLI.js approach is valid because we dont want to dilute the principles of configmgr schema validation, but we also don't want to lose features by forcing things to use a YAML when not required. So, yeah, the command variant that uses the YAML should do configmgr validation, and the one that doesnt could be generic quickJS code.

@MarkAckert
Copy link
Member

I would like to take a closer look, but at a glance, I prefer option (1) or (2). The split CLI.js approach is valid and makes sense, but I'm a little worried about maintenance over time if we're essentially duplicating the bulk of the implementation code in two places, one with configmgr validation and one without. DRY, code drift, and all that. For option (3), my question is: is the benefit to developers worth the additional code maintenance? Or is there another way to make this developer friendly - like zwe install --ds-prefix my-prefix --dummy-config (-dc)?

@Martin-Zeithaml
Copy link
Contributor Author

Martin-Zeithaml commented Oct 17, 2024

Split

The split (so far) was done in the minimal way:

  • cliPrefix - takes std.getenv("ZWE_CLI_PARAMETER_DATASET_PREFIX") and checks it with regex (this is enhancement, current install is not checking it)
  • cliYaml - takes zowe.setup.dataset.prefix, zowe.runtimeDirectory and zowe.environments.jclHeader
    • prefix already checked by schema, we just checking if present
    • zowe.runtimeDirectory and zowe.environments.jclHeader could be undefined, we don't care
  • index.js - the install process
    • We have prefix from parameter or yaml
    • We might have zowe.runtimeDirectory and zowe.environments.jclHeader
      • Checks/manipulation done here
    • Dataset checks, --allow-overwrite logic, --dry-run logic, JCL update and submit

The CLIs together are approx 20 lines, the majority of the code is common in the index.js

Tmp config

We can create in tmp directory config from --ds-prefix, update zwe variables as needed and continue with "config only" code path. I will try to code some POC.

Edit

POC is working, we get the benefit of schema validation of DSN, but also this error, if DSN is not valid:

/zowe/runtime/3000/bin: ./zwe install --ds-prefix '1'
Error: Validation of FILE(/tmp/tmp-9337.yaml):FILE(/zowe/runtime/3000/files/defaults.yaml) against schema /zowe/runtime/3000/schemas/zowe-yaml-schema.json:/zowe/runtime/3000/schemas/server-common.json found invalid JSON Schema data
Validity Exceptions(s) with object at
  Validity Exceptions(s) with object at /zowe
    Validity Exceptions(s) with object at /zowe/setup
      Validity Exceptions(s) with object at /zowe/setup/dataset
        type 'integer' not permitted at /zowe/setup/dataset/prefix; expecting type 'string'

This might be confusing for users, if you don't specify yaml, but you have an error there.

@JoeNemo
Copy link
Contributor

JoeNemo commented Oct 23, 2024

If configmgr is mandatory (which it should be IMHO), then implicit uses of it for defaults might need more user-friendly failure messages. Some users don't know that they are using YAML behind the scenes and that it is getting validated by JSON Schema

@1000TurquoisePogs
Copy link
Member

Could we work around this edge case of zwe install encountering schema validation errors by validating the "--ds-prefix" input in the shell script that runs directly prior to configmgr being invoked?

@Martin-Zeithaml
Copy link
Contributor Author

Yes, of course. If we decide to use #4057, we can test --ds-prefix in shell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Config Manager Related to the config manager component zwe
Projects
None yet
Development

No branches or pull requests

4 participants