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

Record lines truncation of target PDS Enhancement #2433

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

pujal0909
Copy link
Contributor

@pujal0909 pujal0909 commented Feb 9, 2025

What It Does

In the scenario when copying from a source PDS to a target PDS where the target does not have enough record lines for a member, initially, it would copy over that member, truncate it's record length, throw a truncation error and would not continue copying over the subsequent members. Now, it will continue copying over the subsequent members and inform the user that member(s)' contents were truncated and they can view the list of members affected in a local file.

How to Test

  1. With the current functionality, copy a source PDS into a target PDS. The target PDS should have a lower record lines value than the source.
  2. Notice that a member with a long record length will be truncated, and subsequent members will not continue to copy over.
  3. Now with the updated functionality, copy the same source PDS into a target PDS with a lower record lines value than the source.
  4. Notice that the user is informed of the truncation and is given a local file of members with this error. The subsequent members were copied over as well.

Review Checklist
I certify that I have:

Additional Comments

Should this technically be a bug fix?
Should the Github security suggestion about using the tmp library be ignored?

@pujal0909 pujal0909 changed the title Truncate lrecl target Record lines truncation error - target data set Feb 9, 2025
Copy link

codecov bot commented Feb 9, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.49%. Comparing base (b91b989) to head (9d8dcce).

Files with missing lines Patch % Lines
packages/zosfiles/src/methods/copy/Copy.ts 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2433      +/-   ##
==========================================
- Coverage   91.50%   91.49%   -0.01%     
==========================================
  Files         641      641              
  Lines       18376    18392      +16     
  Branches     3872     3877       +5     
==========================================
+ Hits        16815    16828      +13     
- Misses       1559     1561       +2     
- Partials        2        3       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pujal0909 pujal0909 changed the title Record lines truncation error - target data set Record lines truncation of target data set Enhancement Feb 10, 2025
@pujal0909 pujal0909 changed the title Record lines truncation of target data set Enhancement Record lines truncation of target PDS Enhancement Feb 11, 2025
@pujal0909 pujal0909 force-pushed the truncate-lrecl-target branch from a439165 to c62d262 Compare February 12, 2025 19:51
Signed-off-by: Pujal <[email protected]>
@pujal0909 pujal0909 marked this pull request as ready for review February 13, 2025 13:24
Copy link

📅 Suggested merge-by date: 2/27/2025

@traeok
Copy link
Member

traeok commented Feb 18, 2025

If we already warn users that some members were truncated from the copy operation, what are your thoughts on listing the affected members as part of the CLI response? Otherwise, users are forced to locate & open the local file to see what members were truncated.

If there were more than, say, 10 members, we could print so many and then add ...and X more at the end of the output, directing the user to the local file to see the rest.

This isn't a request for changes, but I wanted to propose this as an option for warning the user - this may also help with identifying the troublesome members when copying within a CI environment.

ZosFilesMessages.datasetCopiedSuccessfully.message + " " +
ZosFilesMessages.membersContentTruncated.message + "\n\n" +
firstTenMembers.join('\n') +
`\n... and ${numMembers} more` +
Copy link
Member

Choose a reason for hiding this comment

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

Could this logic result in the message "and 0 more" ?

Copy link
Contributor Author

@pujal0909 pujal0909 Feb 20, 2025

Choose a reason for hiding this comment

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

It would. Should that part of the response be removed for the case of when there are not more than ten truncated members? I wasn't sure if the inconsistent response would be confusing for users.

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely a judgement call. If I were to hit the edge-case of exactly 10, I would think it odd to be told that "0 more" occurred. Alternatively if someone were automating on the output, it might be useful to know that "x more" would always be part of the message. I lean to removing the message when it would display "0 more". Perhaps wait a bit to see if others offer opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree that adding "...and X more" should only be included if X is greater than 1

Signed-off-by: Pujal <[email protected]>
const truncatedMembersFile = path.join(tmpdir(), 'truncatedMembers.txt');
if(truncatedMembers.length > 0) {
const firstTenMembers = truncatedMembers.slice(0, 10);
fs.writeFileSync(truncatedMembersFile, truncatedMembers.join('\n'));

Check failure

Code scanning / CodeQL

Insecure temporary file High

Insecure creation of file in
the os temp dir
.
it("Should copy a partitioned data set", async () => {
let error;
let response;

const truncatedMembersFile = path.join(tmpdir(), 'truncatedMembers.txt');
fs.writeFileSync(truncatedMembersFile, "");

Check failure

Code scanning / CodeQL

Insecure temporary file High test

Insecure creation of file in
the os temp dir
.
Signed-off-by: Pujal <[email protected]>
Copy link
Contributor

@ATorrise ATorrise left a comment

Choose a reason for hiding this comment

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

agreeing with your discussion with Gene that your truncation response should only include an attached file if truncated items list is greater than 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

4 participants