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 issue #156 to return proper message for active job #157

Merged
merged 2 commits into from
Apr 29, 2024
Merged

Conversation

tiantn
Copy link
Collaborator

@tiantn tiantn commented Apr 23, 2024

What It Does

How to Test

Review Checklist
I certify that I have:

Additional Comments

Signed-off-by: Tian Na <tiantn@cn.ibm.com>
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.

Tested locally and looks good to me, thanks @tiantn!

@traeok traeok linked an issue Apr 24, 2024 that may be closed by this pull request
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.

Approve. But trailing space at line 80 in src/api/JobUtils.ts?

Copy link
Member

@t1m0thyj t1m0thyj 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! Tested locally and it works well 👍

$ zowe zftp ls sfbj TSU06008  
No spool file available.
$ zowe zftp view asbj TSU06008 
Command Error:
No spool files were available for job TSU06008. Try again after waiting a moment if the job is not yet in OUTPUT status.

fullSpoolFiles.push(spoolFileToDownload);
if (jobDetails.spoolFiles) {
for (const spoolFileToDownload of jobDetails.spoolFiles) {
this.log.debug("Requesting spool files for job %s(%s) spool file ID %d",
Copy link
Member

Choose a reason for hiding this comment

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

Please remove trailing spaces to fix lint warning

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.

LGTM! 😋
I don't think I have anything to add that other reviewers haven't mentioned 😋

Comment on lines +78 to +79
if (jobDetails.spoolFiles) {
for (const spoolFileToDownload of jobDetails.spoolFiles) {
Copy link
Member

@zFernand0 zFernand0 Apr 25, 2024

Choose a reason for hiding this comment

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

Wondering if we should push the condition down to the for-of loop.
For example for (const spoolFileToDownload of jobDetails.spoolFiles ?? []) {

I'm not really requesting for this to be changed, it's just something that you may want to do in the future if you get lazy (like me) and don't want to bother with indentation 😋

Signed-off-by: Tian Na <tiantn@cn.ibm.com>
Copy link

sonarcloud bot commented Apr 26, 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

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 72.80%. Comparing base (9497b0f) to head (0cea560).
Report is 8 commits behind head on master.

Files Patch % Lines
.../spool-files-by-jobid/SpoolFilesByJobid.Handler.ts 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
- Coverage   73.04%   72.80%   -0.25%     
==========================================
  Files          80       80              
  Lines         983      989       +6     
  Branches      127      131       +4     
==========================================
+ Hits          718      720       +2     
+ Misses        265      248      -17     
- Partials        0       21      +21     

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

@tiantn
Copy link
Collaborator Author

tiantn commented Apr 26, 2024

Thank you all for your good advice. I just removed the trailing spaces. 😊

@JTonda JTonda merged commit 2590044 into master Apr 29, 2024
17 checks passed
@JTonda JTonda deleted the spoolfiles branch April 29, 2024 15:07
@JTonda JTonda added the release-patch Indicates a patch to existing code has been applied label Apr 29, 2024
Copy link

Release succeeded for the master branch. 🎉

The following packages have been published:

  • npm: @zowe/zos-ftp-for-zowe-cli@2.1.9

Powered by Octorelease 🚀

@zFernand0 zFernand0 mentioned this pull request Jun 3, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-patch Indicates a patch to existing code has been applied released
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

findJobByID does not return spoolFiles
7 participants