-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix(V3): Edit team config fails with global & project level present #3138
Conversation
📅 Suggested merge-by date: 10/7/2024 |
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.
LGTM! thanks for the fix @SanthoshiBoyina1
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.
Will approve once Trae's comment is addressed - creating zowe.config.user.json
files should not be the default behavior, as the purpose of User Config is solely to override values in zowe.config.json
.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3138 +/- ##
==========================================
- Coverage 92.79% 92.79% -0.01%
==========================================
Files 113 113
Lines 11663 11669 +6
Branches 2593 2501 -92
==========================================
+ Hits 10823 10828 +5
- Misses 838 839 +1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
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.
left a small comment on the changelog 😅
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
e5f0470
to
cf1e708
Compare
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Hey all, I tried a different approach. Let me know what you think @traeok @t1m0thyj @zFernand0 Edit: sorry, not to look like I'm just barging in on the PR. Santhoshi asked me to help look into resolving the concerns raised in PR. |
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.
LGTM! 😋
Left a small comment about an edge-case. 😅
But it's a bit rare for someone to want to update their config by opening their zoweDir and selecting project as their choice 😋
Signed-off-by: SanthoshiBoyina1 <142206957+SanthoshiBoyina1@users.noreply.github.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
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 great, thanks @SanthoshiBoyina1!
await this.openConfigFile(file.path); | ||
} | ||
} | ||
Gui.showMessage(this.manualEditMsg); | ||
break; | ||
case "global": | ||
for (const file of existingLayers) { | ||
if (file.global) { | ||
if (file.path.includes(FileManagement.getZoweDir())) { |
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 think the change on this line could be reverted to address Fernando's comment 😋
if (file.path.includes(FileManagement.getZoweDir())) { | |
if (file.global) { |
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.
As Timothy mentioned, it looks like we'll need to change the logic back for case "global"
so that it handles opening files in the Zowe dir as well.
Will approve once resolved 😋
Signed-off-by: SanthoshiBoyina1 <142206957+SanthoshiBoyina1@users.noreply.github.com>
Signed-off-by: Santhoshi Boyina <Santhoshi.Boyina1@ibm.com>
|
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.
LGTM! thanks @SanthoshiBoyina1 for the fix
Proposed changes
Solves #3125
To fix Edit team config fails with global & project level present
Release Notes
Milestone:
Changelog:
Types of changes
Checklist
General
yarn workspace vscode-extension-for-zowe vscode:prepublish
pnpm --filter vscode-extension-for-zowe vscode:prepublish
Code coverage
Deployment
Further comments