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

fix list spool file hang problem. #159

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Conversation

tiantn
Copy link
Collaborator

@tiantn tiantn commented Jun 3, 2024

What It Does

How to Test

Review Checklist
I certify that I have:

Additional Comments

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (next@4dea27d). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             next     #159   +/-   ##
=======================================
  Coverage        ?   73.49%           
=======================================
  Files           ?       77           
  Lines           ?      981           
  Branches        ?      128           
=======================================
  Hits            ?      721           
  Misses          ?      240           
  Partials        ?       20           

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

@t1m0thyj t1m0thyj linked an issue Jun 3, 2024 that may be closed by this pull request
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

This looks similar to the following PR:

Thanks for porting and enhancing the functionality and error message.

Curious if we can link the same issue on the changelog (issue number #156)

@@ -23,17 +23,22 @@ export default class ListSpoolFilesByJobidHandler extends FTPBaseHandler {
this.log.debug("Listing spool files for job ID %s", params.arguments.jobId);
const job = await JobUtils.findJobByID(params.connection, params.arguments.jobId);
const files = job.spoolFiles;
if (files) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we check the length as well?
if (files?.length > 0) { ...

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tiantn for the fix!

Copy link
Collaborator

@std4lqi std4lqi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @tiantn !

Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tiantn
Copy link
Collaborator Author

tiantn commented Jun 4, 2024

This looks similar to the following PR:

* [Fix issue #156 to return proper message for active job #157](https://github.com/zowe/zowe-cli-ftp-plugin/pull/157)

Thanks for porting and enhancing the functionality and error message.

Curious if we can link the same issue on the changelog (issue number #156)

Hi @zFernand0, I think it fixed the issue #158 😊

@tiantn
Copy link
Collaborator Author

tiantn commented Jun 4, 2024

Hi @zFernand0 , @traeok , @std4lqi , thank you for your review and good suggestions. I updated the code to check the files length as well. And also update unit test for no spool file scenario to improve test cover rate. Thank you!

@zFernand0
Copy link
Member

Merging without DCO check. See below issue for details 😋

Kudos to @traeok

@zFernand0 zFernand0 merged commit 59c5773 into next Jun 4, 2024
16 checks passed
@zFernand0 zFernand0 deleted the fix-list-spoolfiles-hang-problem branch June 4, 2024 18:16
@zFernand0 zFernand0 added the release-current Indicates that there is no new functionality being delivered label Jun 4, 2024
Copy link

github-actions bot commented Jun 4, 2024

Release succeeded for the next branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-current Indicates that there is no new functionality being delivered released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to view or list spool files for job with "SYS FAIL" RC
5 participants