- 
                Notifications
    You must be signed in to change notification settings 
- Fork 16
show-bundle-in-uss-dir #473
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
base: main
Are you sure you want to change the base?
show-bundle-in-uss-dir #473
Conversation
09c0bff    to
    cb24f53      
    Compare
  
    Signed-off-by: Khushboo <[email protected]>
cb24f53    to
    fe8e7ae      
    Compare
  
    | Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
+ Coverage   68.24%   68.47%   +0.22%     
==========================================
  Files         165      166       +1     
  Lines        3883     3949      +66     
  Branches      549      562      +13     
==========================================
+ Hits         2650     2704      +54     
- Misses       1233     1245      +12     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo Sharma <[email protected]>
        
          
                packages/vsce/__tests__/__unit__/commands/showBundleDirectoryCommand.unit.test.ts
              
                Fixed
          
            Show fixed
            Hide fixed
        
      Signed-off-by: Khushboo Sharma <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Signed-off-by: Khushboo <[email protected]>
Removed a problematic test case that was causing issues. Signed-off-by: Khushboo Sharma <[email protected]>
| I was considering writing an end-to-end test for this, but since the USS tree isn’t part of our extension, I couldn’t find the API calls related to it. I don’t think it’s feasible for us to write an e2e test in this case. From what I observed, they seem to be using environment variables to load data and possibly relying on real test data. Let me know your thoughts.@AndrewTwydell @davenice | 
| 
 Hi Khushboo, for an e2e test we'd be mocking exactly what happens on the wire, using wiremock. So we'd look at which URLs were requested from wiremock, and then mock those using httpie or similar to get the date. We'd want to mock the minimum of their REST API that we could get away with! You could investigate by adding localhost:8080, or wherever your wiremock is listening, as a zosmf connection and seeing which zosmf calls wiremock complains it doesn't have a mock for. You might need to start wiremock in verbose mode to see full details, I can't quite remember! | 
| 
 I can set up a WireMock server for z/OSMF, but adding REST API mappings is challenging because we can’t trace the z/OSMF calls using a TCP/IP debugger—the connection uses HTTPS. | 
| That does make it a bit harder. I think we could use the same wiremock server. If you add your wiremock server as a zosmf connection and then try to use it, you will see from the wiremock output which URL was requested and failed. Then you could request the same kind of URL on a real server to see what comes back to put in your mock. Another option would be to find a way to add some debug to Zowe explorer to output the request before it's made, but this sounds quite a lot of work for what I suspect will only be a few requests to be mocked. Remember that we don't need a fully working wiremock, we don't even need the whole scenario to work properly. We just want to make sure that the call we make to Zowe explorer triggers zosmf to look for information correctly. This protects us against them changing their API in the future and breaking us. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well! Couple of suggestions 👍🏼
| }, | ||
| { | ||
| "command": "cics-extension-for-zowe.showBundleDirectory", | ||
| "when": "config.zowe.cics.showAllCommandsInPalette" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use this feature toggle anymore...
| "when": "config.zowe.cics.showAllCommandsInPalette" | |
| "when": "never" | 
| return; | ||
| } | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param connectionType - The type of connection to check for ('uss' or 'jes'), case-insensitive | ||
| * @returns True if the profile supports the specified connection type, false otherwise | ||
| */ | ||
| export function doesProfileSupportConnectionType(profile: IProfileLoaded, connectionType: string): boolean { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could strongly type this so the developer calling it can ctrl + space to autocomplete - keeps things type safe 😄
| export function doesProfileSupportConnectionType(profile: IProfileLoaded, connectionType: string): boolean { | |
| export function doesProfileSupportConnectionType(profile: IProfileLoaded, connectionType: "JES" | "USS"): boolean { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ZE have a ZoweExplorerApiType enum that we could use in @zowe/zowe-explorer-api...
| export function doesProfileSupportConnectionType(profile: IProfileLoaded, connectionType: string): boolean { | |
| export function doesProfileSupportConnectionType(profile: IProfileLoaded, connectionType: ZoweExplorerApiType): boolean { | 
So the method call would be
doesProfileSupportConnectionType(myProfile, ZoweExplorerApiType.Jes);
doesProfileSupportConnectionType(myProfile, ZoweExplorerApiType.Uss);
...
switch (type) {
      case ZoweExplorerApiType.Uss:
        explorerApi.getUssApi(profile);
        break;
...
| window.showErrorMessage(`No Bundle is selected from cics tree`); | ||
| return; | ||
| } | ||
| const bundleDir = selectedBundle.getContainedResource().resource.attributes?.bundledir; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contained Resource is optional, but attributes is not
| const bundleDir = selectedBundle.getContainedResource().resource.attributes?.bundledir; | |
| const bundleDir = selectedBundle.getContainedResource()?.resource.attributes.bundledir; | 
| } | ||
| const bundleDir = selectedBundle.getContainedResource().resource.attributes?.bundledir; | ||
| if (!bundleDir) { | ||
| window.showErrorMessage(`Could not find bundle directory for ${selectedBundle.getLabel() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| window.showErrorMessage(`Could not find bundle directory for ${selectedBundle.getLabel() | |
| window.showErrorMessage(`Could not find bundle directory for ${selectedBundle.getContainedResourceName() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The label could have other info in that's not necessary here, status info for example
| if (!chosenProfileName) { | ||
| window.showErrorMessage("Could not find any profiles that will access USS (for instance z/OSMF)."); | ||
| return; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am prompted for a profile because we went into line 78, and I clicked esc to cancel when choosing, I then get this error message which shouldn't be the case. If a user cancels out of an action, it should just exit quietly.
What It Does
#474
Review Checklist
I certify that I have:
tested my changes
added/updated automated tests
updated the changelog
considered whether the docs need updating
followed the contribution guidelines