-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add encoding to Submit Jobs CLI and APIs (local-file and stdin) #2139
Conversation
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2139 +/- ##
=======================================
Coverage 91.12% 91.13%
=======================================
Files 636 636
Lines 19040 19046 +6
Branches 4011 3947 -64
=======================================
+ Hits 17351 17357 +6
Misses 1688 1688
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Andrew!
I mentioned the possibility of an extender using something like keyof ISubmitParms
to accept certain key names in a function. Since the changes still keep all existing properties in the same interfaces, this should not be breaking. Once the new keys are available, extenders can choose to handle them at their own discretion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, thanks @awharn! Left a few comments
--job-record-length | --jrl (number) | ||
|
||
The logical record length of the submitted JCL. Defaults to 80 if not supplied. | ||
|
||
--job-record-format | --jrf (string) | ||
|
||
The record format of the JCL being submitted, where V is variable, and F is | ||
fixed. Defaults to F if not supplied. | ||
|
||
Allowed values: F, V |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set defaultValue
on the command option definitions for --job-record-length
and --job-record-format
? This would automatically show the default value in CLI help and the sentence "Defaults to ..." could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the defaults I listed are the z/OSMF defaults if you don't send anything. I wasn't sure if we just wanted z/OSMF to handle it, or if we want to copy their default into the CLI.
--job-record-length | --jrl (number) | ||
|
||
The logical record length of the submitted JCL. Defaults to 80 if not supplied. | ||
|
||
--job-record-format | --jrf (string) | ||
|
||
The record format of the JCL being submitted, where V is variable, and F is | ||
fixed. Defaults to F if not supplied. | ||
|
||
Allowed values: F, V |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above for setting default values
packages/zosjobs/src/SubmitJobs.ts
Outdated
@@ -152,6 +159,9 @@ export class SubmitJobs { | |||
const extraHeaders = this.getSubstitutionHeaders(parms.jclSymbols); | |||
headers.push(...extraHeaders); | |||
} | |||
if (parms.internalReaderFileEncoding) { | |||
headers.push({"X-IBM-Intrdr-File-Encoding": parms.internalReaderFileEncoding}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the header name as a constant in ZosmfHeaders
so we don't need to hardcode it here?
zowe-cli/packages/core/src/rest/ZosmfHeaders.ts
Lines 19 to 34 in e8518da
export class ZosmfHeaders { | |
/** | |
* lrecl header | |
* @static | |
* @memberof ZosmfHeaders | |
*/ | |
public static readonly X_IBM_INTRDR_LRECL = "X-IBM-Intrdr-Lrecl"; | |
/** | |
* recfm header | |
* @static | |
* @memberof ZosmfHeaders | |
*/ | |
public static readonly X_IBM_INTRDR_RECFM = "X-IBM-Intrdr-Recfm"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but this is the only place that we use that header.
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @awharn!
…oding Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Quality Gate failedFailed conditions |
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
Adds additional options for jobs submitted from a user's computer, such as:
Removes duplication among the Jobs interfaces in a non-breaking way
Resolves #742 (--job-encoding IBM-037)
Resolves #876 (--job-encoding IBM-1147)
How to Test
Review Checklist
I certify that I have:
Additional Comments