-
Notifications
You must be signed in to change notification settings - Fork 87
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 error using session with auth type "none" #2218
Conversation
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2218 +/- ##
==========================================
- Coverage 91.16% 91.15% -0.01%
==========================================
Files 638 638
Lines 19103 19103
Branches 3927 3974 +47
==========================================
- Hits 17415 17414 -1
- Misses 1687 1688 +1
Partials 1 1 ☔ 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.
The change makes sense to me conceptually speaking.
I may need some help understanding it a bit better. 🙏🏽
LGTM though 😋
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.
Changes look good to me, thanks Timothy for the quick fix
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
b9b6b5e
to
11a1830
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.
amazing!
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.
Had a bit more time to test.
Thanks @t1m0thyj for helping! 🙏🏽
LGTM! 😋
Tested in 7.24.0 and this PR and they work as expected (no errors thrown).
Tested with 7.24.1 - a couple of versions in between - 7.28.2, and the latest vNext version and we get the error 😋
Also did some testing with varios Zowe bundles
2.9 - 2.15 (versions before CLI-7.24.1) and they all worked 😋
Then, 2.16 started to fail (as expected)
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 agree with the implementation using AUTH_TYPE_NONE. AUTH_TYPE_NONE does not fall into our order-of-precedence. Its presence is an indicator that the items in our order-of-precedence do not even apply to the action.
I suppose AUTH_TYPE_NONE could be tested earlier and we could skip many of the credential checks, but then we would still have to set the headers and flags at the end of the function. Placing the test for AUTH_TYPE_NONE on an existing test (as you have done) minimizes the changes that have to be made, and may even make the logic easier for a human to follow.
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Quality Gate passedIssues Measures |
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
Fixes #2219
How to Test
Try issuing a REST request with no credentials on the session object:
Review Checklist
I certify that I have:
Additional Comments