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

show logs initial implementation #237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

davenice
Copy link
Contributor

@davenice davenice commented Feb 24, 2025

What It Does
Addresses #56

Review Checklist
I certify that I have:

  • tested my changes
  • added/updated automated tests
  • updated the changelog
  • updated the readme
  • followed the contribution guidelines

Additional Comments

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 40.99%. Comparing base (2d13589) to head (dd807f7).

Files with missing lines Patch % Lines
packages/vsce/src/commands/showLogsCommand.ts 60.00% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   40.78%   40.99%   +0.20%     
==========================================
  Files         151      152       +1     
  Lines        5056     5111      +55     
  Branches      817      874      +57     
==========================================
+ Hits         2062     2095      +33     
- Misses       2994     3016      +22     

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

@davenice davenice force-pushed the niced-open-logs-for-region branch from 5b2c38f to f18c534 Compare February 24, 2025 18:12
Comment on lines 21 to 36
export async function findRelatedZosProfiles(cicsProfile: IProfileLoaded, zosProfiles: IProfileLoaded[]): Promise<IProfileLoaded> {
const baseForCicsProfile = await ProfileManagement.getProfilesCache().fetchBaseProfile(cicsProfile.name);

// find all zosmf and rse profiles (put zosmf first)
zosProfiles = zosProfiles.sort((prof) => (prof.profile.type === "zosmf" ? -1 : 1));

// filter out profiles that are not in the same base as the cics profile
const matchingProfiles: IProfileLoaded[] = [];
if (baseForCicsProfile) {
for (const profile of zosProfiles) {
if (baseForCicsProfile && baseForCicsProfile?.name === (await ProfileManagement.getProfilesCache().fetchBaseProfile(profile.name))?.name) {
matchingProfiles.push(profile);
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JillieBeanSim / @zFernand0 - this is the part of the PR I'm particularly interested in your thoughts on.

I'm taking a CICS profile and a list of proposed zosProfiles.

I want to identify a zosProfile that matches the CICS profile. I'm currently doing this by requesting the base profile for each proposed zosProfile to see if it matches the base profile of the cicsProfile.

Is this the best way, currently?

Choose a reason for hiding this comment

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

For Profiles that match a particular CICS profile you may want to use ProfilesCache.fetchAllProfiles() then filter down to the profiles with matching host. Also, depending on the scenario you may want to filter even further on profile types you may want to support removing types like TSO, SSH or what doesn't fit. All these profiles will already have any merging done if needed, relying on base profile can prove to be troublesome since users configure their Zowe Config in different ways and sometimes without base profiles.

Choose a reason for hiding this comment

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

if you know the profile type there is also ProfilesCache.fetchAllProfilesByType(type) returning complete/merged profiles

Copy link
Contributor Author

@davenice davenice Feb 28, 2025

Choose a reason for hiding this comment

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

Thanks Billie - I started with fetchAllProfilesByType, running it twice for z/OSMF and RSE but decided it was easier to just fetch all profiles and filter within our code (

const zosProfiles = (await ProfileManagement.getProfilesCache().fetchAllProfiles()).filter((profile) => ["zosmf", "rse"].includes(profile.type));
)

So zooming out a bit the logic I've used is:

  1. Try and use the base profile to identify a connection within the same nested group (
    if (baseForCicsProfile && baseForCicsProfile.name === (await ProfileManagement.getProfilesCache().fetchBaseProfile(profile.name))?.name) {
    )
  2. If not found, find a connection that uses the same hostname (
    const cicsHostProfiles = zosProfiles.filter((profile) => cicsProfile.profile.host === profile.profile.host);
    )
  3. If not found, offer the user all z/OSMF and RSE connections to pick from (
    placeHolder: vscode.l10n.t("Select a profile to access the logs"),
    )

So I think the summary is that you're saying there isn't a better way :-( , and that the code does deal with your concerns, which is good :-)

Let me know if anything else occurs. 👍

@davenice davenice force-pushed the niced-open-logs-for-region branch from 4ac2de7 to 57dee9e Compare February 27, 2025 10:07
Copy link
Contributor

@AndrewTwydell AndrewTwydell left a comment

Choose a reason for hiding this comment

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

Looking good! Only some suggestions because you asked so nicely for some 😄

@davenice
Copy link
Contributor Author

davenice commented Feb 27, 2025

Here is a quick vid showing what this does and why it's not exactly what @Joe-Winchester suggested in the original issue :-)

Happy, @Joe-Winchester ?

(edit: whoops - uploaded the wrong version first time - this one has sound where I explain the scenarios I show!)
https://github.com/user-attachments/assets/7f3e02e9-e539-4767-aad4-9fd4852ad046

@davenice davenice marked this pull request as ready for review February 27, 2025 19:18
davenice and others added 3 commits February 27, 2025 21:37
linting
changelog
more unit tests
don't offer zos profiles that aren't authenticated

Signed-off-by: Dave Nice <[email protected]>
Co-authored-by: Andrew <[email protected]>
Signed-off-by: Dave Nice <[email protected]>
Signed-off-by: Dave Nice <[email protected]>
@davenice davenice force-pushed the niced-open-logs-for-region branch from d1f3e9b to dd807f7 Compare February 27, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

4 participants