feat: Data set and USS download commands#3843
Conversation
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive download functionality for data sets and USS files/directories to Zowe Explorer. It introduces 5 new commands with extensive configuration options including encoding selection, directory filtering, and overwrite protection.
Key changes:
- USS file and directory download with filter options (owner, permissions, size, mtime, depth, type)
- Data set and member download with extension mapping and directory generation
- Encoding selection including auto-detect for USS directories and record format support
- Detailed download response handling with error/warning reporting
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| USSActions.ts | Implements USS file/directory download with filter options and encoding selection |
| DatasetActions.ts | Implements data set/member download with extension mapping and record format support |
| SharedUtils.ts | Adds download response handler and directory encoding prompts |
| DatasetUtils.ts | Adds extension mapping for PDS members |
| MainframeInteraction.ts | Extends API interface with downloadDirectory and downloadAllMembers |
| ZoweExplorerZosmfApi.ts | Implements new download APIs for z/OSMF |
| FTP extension files | Adds stubs for unsupported download operations |
| Test files | Comprehensive unit test coverage for new functionality |
| Configuration files | Adds storage keys and constants for download options |
The implementation is well-structured with proper separation of concerns, comprehensive test coverage, and follows established patterns in the codebase. The code handles various edge cases including encoding detection, large file warnings, and error scenarios appropriately.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
| * | ||
| * @param {string} ussFilePath | ||
| * @param {zosfiles.IDownloadOptions} options | ||
| * @returns {Promise<zosfiles.IZosFilesResponse>} |
There was a problem hiding this comment.
Thanks for adding the return value to the TSDoc. 🙏
It would be nice if all functions were consistently returning the proper Promise<...> value 😅
This is not a request for changes though 😋
There was a problem hiding this comment.
Confused on this one. Which ones aren't returning the proper Promise<...> value? Thanks
packages/zowe-explorer/__tests__/__unit__/trees/dataset/DatasetUtils.unit.test.ts
Show resolved
Hide resolved
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadAllMembers", async (node: IZoweDatasetTreeNode) => | ||
| DatasetActions.downloadAllMembers(node) | ||
| ) | ||
| ); | ||
|
|
||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadMember", async (node: IZoweDatasetTreeNode) => DatasetActions.downloadMember(node)) | ||
| ); | ||
|
|
||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadDataSet", async (node: IZoweDatasetTreeNode) => DatasetActions.downloadDataSet(node)) | ||
| ); |
There was a problem hiding this comment.
dumb question: 🤚
Should we await these calls?
There was a problem hiding this comment.
No idea, does it matter? It already returns a promise, the await wouldn't change anything other than be more explicit I suppose? But please, not dumb question, it made me question reality. I am likely missing something in your motivation to ask 😅
There was a problem hiding this comment.
I would compare with others that are async which usually do have an await but if we would like it to run in background then no await, would just need to make sure the popup message for success isn't shown before the download is complete I would think
There was a problem hiding this comment.
My understanding is that vscode commands are always aysnchronous so it doesn't matter if there is an await or not, the downloads will always run in the background and not block any other commands from being executed. I did try it with an await but there is no difference in functionality. However I think I will add an await regardless because apparently it should give a better stack trace if things go wrong?
| if (ext === ".c") { | ||
| // Special case for ".c" extension, skip the following logic | ||
| // As it's not unique enough and would otherwise match on anything containing "C" | ||
| // TODO: rrevisit later if this can be handled better while still allowing c files | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Thanks for adding this TODO comment.
Should we create an issue about this?
|
|
||
| // If no extension found, fall back to using the PDS name as extension | ||
| if (!extension) { | ||
| const parentExtension = DatasetUtils.getExtension(node.label as string); |
There was a problem hiding this comment.
Curious if we should handle the .c pattern-matching in the getExtension function?
That way we can avoid some code duplication (i.e. the regex match done in the section above. 😋
It'd be good to discuss, as such change may be considered breaking. 🙏
There was a problem hiding this comment.
Eeeh, yeah I didn't want to touch getExtension because of it being breaking changes. Maybe an issue you are right, but probably a V4 thing?
| if (SharedContext.isUssDirectory(child)) { | ||
| if (maxDepth === undefined || currentDepth < maxDepth) { | ||
| totalCount += await this.countAllFilesRecursively(child, maxDepth, currentDepth + 1); | ||
| } | ||
| } else { | ||
| totalCount += 1; | ||
| } | ||
|
|
||
| // Return early to avoid unnecessary recursion | ||
| if (totalCount > Constants.MIN_WARN_DOWNLOAD_FILES) { | ||
| return totalCount; | ||
| } |
There was a problem hiding this comment.
I love this! 🙏
I do have one request though.
Could we pass the current value of totalCount to the deeper calls?
Not to add it, but to check for the totalCount > (warning_number = 100)
Consider the scenario of me downloading this "dummy" folder:
- dumy
- A01
- A02
...- A99
- B (directory)
- B01
- B02
...- B99
- C (directory)
- C01
...
There was a problem hiding this comment.
This is a good suggest, but honestly I don't think there is really a need for this function anymore. It was my first idea to count number of files in tree to warn of max files, but then when I added support for filters the count would be come entirely inaccurate so I swapped to doing the call on list API with this recursive count as a "fallback". But if the list API call fails then we probably have bigger problems than trying to do a recursive count then a download API call which would probably also fail.
There was a problem hiding this comment.
@zFernand0 would be you opposed if I yeeted this function out of existence? due to the above comment. thanks
There was a problem hiding this comment.
I'm just going to do it because it's a useless function now.
There was a problem hiding this comment.
Hey 👋
Here is a smaller round of comments. 😅
I'll wait for some of the comments to be resolved before I continue reviewing 🙏
Note: I still have some files left to review (mostly around unit tests and UI items)
Apologies for waiting so long to review this 😢
Requested changes:
- Take action on unused functions
- Please consider not forcing
auto-detectas a string/hardcoded type
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
|
Hi Josh, I found the vids you posted before really helpful. I downloaded this just now to have another play. Generally I think it's great. I haven't had a huge need to manipulate files like this but I think it would replace some use of scp/ftps for me. Thinking about the UX, which is where you had more of a concern, here are my thoughts: For data sets
For USS
I'm not certain about any of these suggestions - it's just food for thought based on my clicking through! (other than the USS behaviour downloading a directory which I feel is incorrect). Thanks for taking so much time and effort to try and get this just right! |
|
Thanks a lot for the feedback again Dave! Data sets
Indeed. I considered having a webview for downloading options but I spoke to some colleagues and they said they'd prefer a quicker way to downloading stuff once they'd selected the options they like from the start. But in hindsight (especially after I discovered and added the USS advanced filter options, the encoding selection and the like) a webview might've been better. Then the "quickest" side of that would've been a command palette command to enter data set qualifier or USS path to download based on their saved settings from webview, but without opening the webview for that specific action. However I am still up for changing all that, because as I say I don't want to ship a feature with bad UX if that's what feedback comes to.
Am using vscode api there, which always defaults to opening in home directory for a given session when you select "open folder".
Yeah thanks for mentioning that. So the reason I have this name is because this is the name the flag has in Zowe CLI. I also got confused by this naming, I don't think it is good at all. If the CLI team are ok with it, I'd prefer to change the option to something else, not sure what, maybe something like "Download in all upper case" or something. Because, if the option is true, all downloaded files and folders will be upper case (like on mainframe). If it is false, all downloaded files and folders will be lower case. USS
Comes back to the above point with having a webview. My thinking is that options could be layed out in one view under their different categories, rather than several layers of quick picks. Again happy to switch to webview idea if that's what people would prefer.
Fair enough, I'll do that.
Maybe useful maybe not. Again it's a flag in Zowe CLI and wouldn't want to mess with people's workflows (those like me who are using CLI currently for downloads because ZE doesn't support it yet) just because we couldn't think of a specific use case of it. The upside of a single layer of quick pick is you can have all these options on it and if someone doesn't care they can leave it unchecked. If its on by default though I will change it to be off by default. Can't remember off the top of my head though.
This is a behaviour of the Zowe CLI not of my frontend implementation, but I do think this is a valid point. I would likely be able to change it to the behaviour that makes more sense that you suggest however again this might "break" someone's workflow and not be consistent with how it's working at CLI level so perhaps best to open an issue for it there after the fact (but it would be a breaking change for V4). Thanks! |
|
Ah I see - having not looked in detail at the code I didn't realise we'd inherited so much of this from Zowe CLI. Thinking about "download directory" not downloading the actual directory you click on I can't imagine there will be much appetite for changing the CLI behaviour at this point but it seems like it would be contrary to cp, scp and other similar CLI tools. Would be good to see what @zFernand0 thinks. |
|
Also I think I'd be in favour of tweaking the names/descriptions in the VSCE and not necessarily having them match the CLI. The VSCE is a more mass market user friendly tool and I think it's a good chance to get things right(er) if needed. People using the CLI as well might be slightly niggled, but they'll get over it. Again we'd want to take a steer from @zFernand0 and crew! |
Oh yeah I remember why I did it this way. It's because these two options are booleans, while the options in the advanced filters are all string or number input. So I would need to code exceptions for those options to not enter an extra layer of quick pick to select their values (because that wouldn't make sense either) so it'd get really messy with all the persistence and stuff. |
JillieBeanSim
left a comment
JillieBeanSim
left a comment
There was a problem hiding this comment.
Thanks for working on this @JWaters02
i have left a few comments/questions and also with the discussion on the UI I wonder if we should keep compatibility with the jobs tree search without the check boxes and clicking on the item takes them to a way to fill in the value. Any boolean values we need to clearly document the default in description and docs.
| * minimal properties name, mode. | ||
| */ | ||
| fileList(ussFilePath: string): Promise<zosfiles.IZosFilesResponse>; | ||
| fileList(ussFilePath: string, options?: zosfiles.IUSSListOptions): Promise<zosfiles.IZosFilesResponse>; |
There was a problem hiding this comment.
brought up in another issue, with adding a new optional parameter should we create a new API and mark the other as deprecated?
There was a problem hiding this comment.
Hmm @JillieBeanSim I do not understand why this would be a breaking change or not backwards compatible, as I thought simply making a parameter optional is enough to get by.
| public fileList(ussFilePath: string, options?: zosfiles.IUSSListOptions): Promise<zosfiles.IZosFilesResponse> { | ||
| return zosfiles.List.fileList(this.getSession(), ussFilePath, { | ||
| responseTimeout: this.profile?.profile?.responseTimeout, | ||
| ...options, |
There was a problem hiding this comment.
would we need to check for existence of options here to avoid issues like seen here or have a new API deprecating the old for adoption of new feature
There was a problem hiding this comment.
In the case of the previous issue where the optional parameter was added, the mistake was that they were using it without checking if it was existent or not. In this case however, the zosfiles.List.fileList API also lists options as optional, so if options does not exist, it does not matter as fileList will continue to work as before but without the specific options. So I don't think any check here needs to be done.
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadAllMembers", async (node: IZoweDatasetTreeNode) => | ||
| DatasetActions.downloadAllMembers(node) | ||
| ) | ||
| ); | ||
|
|
||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadMember", async (node: IZoweDatasetTreeNode) => DatasetActions.downloadMember(node)) | ||
| ); | ||
|
|
||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand("zowe.ds.downloadDataSet", async (node: IZoweDatasetTreeNode) => DatasetActions.downloadDataSet(node)) | ||
| ); |
There was a problem hiding this comment.
I would compare with others that are async which usually do have an await but if we would like it to run in background then no await, would just need to make sure the popup message for success isn't shown before the download is complete I would think
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Thanks for the review @JillieBeanSim in terms of the jobs search UI part which part of the UI are you specifically talking about? The USS advanced filters part? I think maybe it might make sense there as currently if you select multiple filters to change it brings up each one in a row, but with jobs search design they would change each part in turn. So it would more clicks/actions but would probably make more sense UX wise. As for the other parts (top level option quick picks) just having the checkboxes makes most sense to me, because they are just booleans and don't need some extra string or number box to specify value. Edit: I think I realised what you meant. I think a better example might be the create data set quick picks - or more specifically, the edit attributes part. I think this UX would work much better than what I currently have for the USS directory filter options. I will implement this I think. Thanks! |
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/zowe-explorer/__tests__/__unit__/trees/uss/USSUtils.unit.test.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
Signed-off-by: JWaters02 <watersjoshua2002@gmail.com>
8 participants
Proposed changes
Adds 5 new commands:
Didn't add command for download all data sets even though its in the API because of #2536 but happy to add it if something thinks it would be useful to have it in ZE...
Data set download demo
https://github.com/user-attachments/assets/843893e2-bcb2-4f56-b3dd-bbf3a5903804
DS.Download.Demo.P2.mp4
USS download demo
https://github.com/user-attachments/assets/aa8250b6-de7e-4c6d-af3c-b407a41c7028
Release Notes
Milestone: 3.5.0
Changelog:
Types of changes
Checklist
General
yarn workspace vscode-extension-for-zowe vscode:prepublishpnpm --filter vscode-extension-for-zowe vscode:prepublishCode coverage
Deployment
Further comments
Please could someone (ideally someone on zowe cli team who worked on downloads api) have a look at the options I am passing into downloads and if I am missing any/getting some wrong etc. Also pretty sure I am handling encoding/tagging wrong in the USS code if someone could look at that. Thanks