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

Issue #969: Jobs with a hash in the name can't be opened #1253

Merged
merged 7 commits into from Apr 21, 2021

Conversation

katelynienaber
Copy link
Contributor

@katelynienaber katelynienaber commented Mar 30, 2021

Proposed changes

It's a fix for Issue #969. I didn't have trouble with the issue mentioned there. But the job output couldn't be opened, because function vscode.Uri.parse was interpreting the # incorrectly.

Fixed by using function vscode.Uri.parse().with() instead, to more explicitly parse the job info as an URI.

Release Notes

Milestone: Backlog
Changelog: Fixed error when showing job output, for jobs that have a hash character (#) in the name.

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

This was reported by at least two customers and should probably be merged ASAP.

Signed-off-by: katelynienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber added this to the Backlog milestone Mar 30, 2021
@katelynienaber katelynienaber self-assigned this Mar 30, 2021
@katelynienaber
Copy link
Contributor Author

@zFernand0 Requesting your review, because there is a vulnerability reported?

In package @zowe/cli...maybe you know how to fix it?

@katelynienaber katelynienaber linked an issue Mar 30, 2021 that may be closed by this pull request
@zFernand0
Copy link
Member

there is a vulnerability reported? (In package @zowe/cli)

I believe updating to the latest version of the CLI is still on our best interest.

Since @zowe/cli@6.23.0 there have been many patches (see changelog.md) to address Imperative known vulnerabilities.

Hopefully the same changelog can help us find any possible quirks the extensions may have. 😋

@zFernand0
Copy link
Member

zFernand0 commented Mar 31, 2021

After a quick update to the entire yarn.lock file, seems like the CLI vulnerabilities have been resolved (on version 6.29.0).
Something we may want to consider is locking down the version of vscode in our devDependencies as this can cause issues when building a package.
For example,

public getLabel(): string {
    return this.label.trim(); 
    // This fails becuase `typeof this.label === 'string | TreeItemLabel'` and there is no `.trim()` on TreeItemLabel 
    // Instead we have to...
    return typeof this.label === "string" ? this.label.trim() : this.label?.label.trim();
}

@VitGottwald, @phaumer, thoughts?

@jellypuno jellypuno added the 21PI1 label Apr 1, 2021
zFernand0 and others added 2 commits April 8, 2021 14:24
Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>
@jellypuno
Copy link
Contributor

@katelynienaber Can you please check the theia tests here as well?

@jellypuno jellypuno modified the milestones: Backlog, 1.14 Release Apr 15, 2021
Copy link
Contributor

@jellypuno jellypuno left a comment

Choose a reason for hiding this comment

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

I know that the change that you have here is very simple but I don't know why you have 8 files edited. Is it because you executed prettify? Is this really a change or was it an accident?

.github/workflows/zowe-explorer-ftp-ci.yml Outdated Show resolved Hide resolved
jellypuno and others added 2 commits April 20, 2021 16:25
Copy link
Contributor

@lauren-li lauren-li 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 good to me! All unit and expected integration tests are passing, and I'm now able to open spool files from a job with a # character in the name.

Thanks @katelynienaber for this fix!

@jellypuno
Copy link
Contributor

@katelynienaber This works for me as well however I am experiencing issues with Integration tests. I have errors in rename and create datasets

15) Extension Integration Tests
       Copying data sets
         Success Scenarios
           Member > Member
             "before each" hook for "Should copy a data set":
     TypeError: Cannot read property 'trim' of undefined

 11) Extension Integration Tests
       Enter Pattern
         should match data sets for multiple patterns:
     Error: Cannot resolve tree item for element 0/0:zcobol /0:No datasets found
  	at c:\Users\jp669971\AppData\Local\Programs\Microsoft VS Code\resources\app\out\vs\workbench\services\extensions\node\extensionHostProcess.js:86:61247
  	at processTicksAndRejections (internal/process/task_queues.js:97:5)
     

Is this also happening to you?

@katelynienaber
Copy link
Contributor Author

@jellypuno those two tests are passing for me, maybe as well for @lauren-li ?
image

image

@jellypuno
Copy link
Contributor

ok then so maybe it is just my set-up. thanks for checking!

@jellypuno jellypuno self-requested a review April 21, 2021 09:48
@jellypuno jellypuno merged commit 7333ff4 into master Apr 21, 2021
@jellypuno jellypuno deleted the fix-job-name-with-hash branch April 21, 2021 10:06
@lauren-li
Copy link
Contributor

@jellypuno those two tests are passing for me, maybe as well for @lauren-li ?
image

image

I had no issues with the copy data set test. The "match data sets for multiple patterns" test always times out for me, even on master branch, so I didn't note it as unusual when it did the same while I was testing this PR branch.

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

Successfully merging this pull request may close these issues.

Jobs view unable to expand job names that have a '#' in them
5 participants