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

v3 pre-release Sept week 2 #3106

Merged
merged 7 commits into from
Sep 13, 2024
Merged

v3 pre-release Sept week 2 #3106

merged 7 commits into from
Sep 13, 2024

Conversation

JillieBeanSim
Copy link
Contributor

@JillieBeanSim JillieBeanSim commented Sep 10, 2024

Proposed changes

v3 pre-release scheduled for week 2 of September

Release Notes

Milestone: v3 pre-release

Changelog: n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Further comments

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim added the no-changelog Add to PR's that don't require a CHANGELOG update label Sep 10, 2024
@JillieBeanSim JillieBeanSim self-assigned this Sep 10, 2024
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim added this to the v3 pre-releases milestone Sep 10, 2024
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.57%. Comparing base (8146202) to head (f5c675a).
Report is 8 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #3106    +/-   ##
========================================
  Coverage   92.57%   92.57%            
========================================
  Files         113      113            
  Lines       11635    11636     +1     
  Branches     2471     2579   +108     
========================================
+ Hits        10771    10772     +1     
  Misses        862      862            
  Partials        2        2            

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

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@@ -150,6 +150,7 @@ export class ProfilesCache {
return;
}
const allTypes = this.getAllProfileTypes(apiRegister?.registeredApiTypes() ?? []);
allTypes.push("ssh");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added for extenders calling loadNamedProfile() or other profile fetching APIs for a SSH profile. possible regression as it previously worked.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea to resolve the regression for now, but curious if we should discuss this a bit more 😋

I guess I'm trying to understand how/when the regression started, since I don't recall us having to add the ssh profile as part of the loaded types.

Personal guess: Maybe because we moved to individual SDKs?

Copy link
Member

@t1m0thyj t1m0thyj Sep 13, 2024

Choose a reason for hiding this comment

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

Also curious why we need to hardcode "ssh" here, since other core profile types ("zosmf" and "tso") seem to work without being manually added to the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zosmf is added to the registered profile list when registered during activation, left comment about double checking tso if it's a regression and possibly updating to register the other types similar to how cics profiles register.

### Bug fixes

- Fixed errors being logged silently rather than thrown in `ProfilesCache.refresh` method. [#3066](https://github.com/zowe/zowe-explorer-vscode/issues/3066)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this changelog entry exists under 2.18.0

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>

- Fixed JSON errors being ignored when `zowe.config.json` files change on disk and are reloaded. [#3066](https://github.com/zowe/zowe-explorer-vscode/issues/3066) [#3074](https://github.com/zowe/zowe-explorer-vscode/issues/3074)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this changelog entry exists under 2.18.0

@JillieBeanSim JillieBeanSim marked this pull request as ready for review September 12, 2024 13:42
Copy link

📅 Suggested merge-by date: 9/26/2024

Signed-off-by: Billie Simmons <BillieJean.Simmons@ibm.com>
Copy link

sonarcloud bot commented Sep 13, 2024

Copy link
Contributor

@likhithanimma1 likhithanimma1 left a comment

Choose a reason for hiding this comment

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

Changes look good to me @JillieBeanSim . Thankyou :)

Copy link
Contributor

@SanthoshiBoyina1 SanthoshiBoyina1 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 for the work @JillieBeanSim

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! 😋
Left a few comments just wondering about the SSH regression.

### Bug fixes

- Fixed errors being logged silently rather than thrown in `ProfilesCache.refresh` method. [#3066](https://github.com/zowe/zowe-explorer-vscode/issues/3066)
- Fix extender's ability to fetch profile information from ProfilesCache for SSH profile types.
Copy link
Member

Choose a reason for hiding this comment

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

Curious if we should add this PR as a link.
Also, wondering how the regression occurred.

@@ -309,7 +309,7 @@ describe("ProfilesCache", () => {
expect((profCache as any).profilesByType.size).toBe(0);
expect((profCache as any).defaultProfileByType.size).toBe(0);
expect((profCache as any).allProfiles.length).toBe(0);
expect((profCache as any).allTypes).toEqual(["base"]);
expect((profCache as any).allTypes).toEqual(["ssh", "base"]);
Copy link
Member

Choose a reason for hiding this comment

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

Would extenders trying to load tso or zosmf profiles have the same prroblem?

@@ -150,6 +150,7 @@ export class ProfilesCache {
return;
}
const allTypes = this.getAllProfileTypes(apiRegister?.registeredApiTypes() ?? []);
allTypes.push("ssh");
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea to resolve the regression for now, but curious if we should discuss this a bit more 😋

I guess I'm trying to understand how/when the regression started, since I don't recall us having to add the ssh profile as part of the loaded types.

Personal guess: Maybe because we moved to individual SDKs?

@JillieBeanSim JillieBeanSim merged commit 3838594 into next Sep 13, 2024
24 checks passed
@JillieBeanSim JillieBeanSim deleted the pre-release/sept-week2 branch September 13, 2024 21:12
@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Sep 13, 2024

we can def look at better way and test the TSO as I have not, it could be due to the change to SDKs. zosmf profiles are registered during ZE activation so included in the registered profiles array. Maybe ssh and tso should get registered as an alternate type like cics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Add to PR's that don't require a CHANGELOG update size/S
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

5 participants