-
Notifications
You must be signed in to change notification settings - Fork 9
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
Now URI encoding the region and cicsplex names #189
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
=======================================
Coverage 93.76% 93.76%
=======================================
Files 75 75
Lines 770 770
Branches 43 42 -1
=======================================
Hits 722 722
Misses 48 48 ☔ View full report in Codecov by Sentry. |
Thanks Enam - one thing worth mentioning is that I have seen a customer using system groups with hashes in - system groups don't support wildcards so they made their own which they maintained manually 😬 I don't think there is a lot of support for system groups in the GUI - the main one I've seen is that if you specify a region in your config, you can specify a system group instead. I think this will work with your changes because I think the code just sees it as a region name rather than a region group - do you think we've covered all the places this might need addressing? |
2ac4b53
to
433f8a9
Compare
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 - few things to look at 😄
6023da4
to
e72e170
Compare
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 👍🏼
8cdf317
to
1ea81dc
Compare
Signed-off-by: EKhan <[email protected]>
Signed-off-by: EKhan <[email protected]>
1ea81dc
to
a9a2e66
Compare
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.
Overall, this LGTM! 😋
I do have one small request to keep the old CMCI.constant to avoid possible breaking changes 🙏
Signed-off-by: EKhan <[email protected]>
e312e4e
to
c18d6b6
Compare
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! 😋
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
Issues was caused by not encoding the # so have updated the getResourceUri method so that both the region and cicsplex names are now uri encoded.
How to Test
Created a region with a # in the name and connected with updated extension. Have also added some unit tests to cover this case.
Review Checklist
I certify that I have:
Additional Comments
Resolves #178