-
Notifications
You must be signed in to change notification settings - Fork 10
SDK getResource failOnNoData flag #205
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
Conversation
a2c3109
to
ec5b1e2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 39.91% 40.04% +0.12%
==========================================
Files 147 148 +1
Lines 5003 5072 +69
Branches 873 891 +18
==========================================
+ Hits 1997 2031 +34
- Misses 3006 3041 +35 ☔ View full report in Codecov by Sentry. |
This looks good too - do we need it for use in VSCE somewhere? failOnNoData is added, but is optional, right? Is there any way this could break existing callers? |
ec5b1e2
to
59dbf7c
Compare
Yeah the idea will be we implement this into the VSCE getResource calls so we don't have to handle NODATA CMCI errors. The fact it's optional means we're not breaking current users of the SDK as it's default behavior is not changing; only once you opt in to the new features using these optional flags will the behaviour change. Currently a WIP, looking at an optional flag to change the error you can get out too which will offer more helpful CMCI info on failures (RESP, RESP2 etc) |
b5a79d9
to
c747d05
Compare
a8badee
to
5b3a07b
Compare
5b0bc58
to
ac10a1a
Compare
The code looks good as far as I can see. I think we normally call it RESP/RESP2 rather than RESPONSE_1 and RESPONSE_2 - so we'd talk about "your RESP code" or "your RESP2 code" - but I don't think that's a big problem and it does disambiguate a bit! I appreciate having a unit test that's identical other than asking for the old and new style of error response. That's useful. Are there any other kinds of errors we ought to drive that way? Is there anything other than "NODATA" we should prod? I will sync the code and test it - but because this is and SDK change it would be valuable to have @zFernand0's input, or somebody that he delegates to! |
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 adding the cmci request options! 🙏
Seem like the failOnNoData is properly defaulted to true
as not to break existing behavior on the CLI 🙏
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.
Two minor wording tweaks and it's a yes from me
Signed-off-by: Andrew Twydell <andrew.twydell@ibm.com>
Signed-off-by: Andrew Twydell <andrew.twydell@ibm.com>
Signed-off-by: Andrew Twydell <andrew.twydell@ibm.com>
Signed-off-by: Andrew Twydell <andrew.twydell@ibm.com>
00bb859
to
a26121b
Compare
What It Does
Adds a failOnNodata flag, defaulting to true (which is the current behaviour), to dictate if CMCI sending a NODATA response should throw an error or not.
Review Checklist
I certify that I have:
Additional Comments