-
Notifications
You must be signed in to change notification settings - Fork 36
Support responseTimeout profile property for z/OSMF operations #369
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
base: main
Are you sure you want to change the base?
Support responseTimeout profile property for z/OSMF operations #369
Conversation
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.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.
Pull Request Overview
Adds support for the responseTimeout profile property by surfacing it on the session object and sending it as an HTTP header for z/OSMF calls.
- Introduces
response_timeoutfield onISession - Assigns
response_timeoutfrom profile props for both auth flows - Includes
X-IBM-Response-Timeoutheader in SDK API requests
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/core/zowe/core_for_zowe_sdk/session.py | Adds response_timeout attribute and assigns from responseTimeout prop |
| src/core/zowe/core_for_zowe_sdk/sdk_api.py | Adds X-IBM-Response-Timeout header using response_timeout |
Comments suppressed due to low confidence (3)
src/core/zowe/core_for_zowe_sdk/sdk_api.py:52
- [nitpick] New header behavior should be covered by automated tests to verify that
X-IBM-Response-Timeoutis only sent when the profile property is set and that its value is correctly formatted.
"X-IBM-Response-Timeout": self.session.response_timeout,
src/core/zowe/core_for_zowe_sdk/session.py:27
- Using
Optional[str]may allow non-numeric values; consider usingOptional[int](or validating/casting the string) so that the timeout is always a valid number.
response_timeout: Optional[str] = None
src/core/zowe/core_for_zowe_sdk/session.py:71
- Directly assigning the raw prop can lead to unexpected types; add validation or cast to the expected type (e.g.,
int(props.get(...))).
self.session.response_timeout = props.get("responseTimeout")
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #369 +/- ##
==========================================
+ Coverage 81.86% 82.01% +0.14%
==========================================
Files 48 49 +1
Lines 2768 2791 +23
==========================================
+ Hits 2266 2289 +23
Misses 502 502
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| self.session.user = props.get("user") | ||
| self.session.password = props.get("password") | ||
| self.session.reject_unauthorized = bool(props.get("rejectUnauthorized")) | ||
| self.session.response_timeout = props.get("responseTimeout") |
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.
@zFernand0 @traeok Is it ok to store response_timeout on the Session class since it's specific to z/OSMF? Wondering if we should keep the properties on the Session class more profile/service-agnostic.
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.
Keeping the core package zosmf agnostic makes sense.
Maybe we can move this response timeout somewhere in the zosmf package?
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.
Thank you for the review, can you review the changes I tried moving it to the zosmf package
pem70
left a comment
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 LGTM.
zFernand0
left a comment
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 make sense.
I agree with Timothy's comment that we should keep the session free from z/somf specific properties
| self.session.user = props.get("user") | ||
| self.session.password = props.get("password") | ||
| self.session.reject_unauthorized = bool(props.get("rejectUnauthorized")) | ||
| self.session.response_timeout = props.get("responseTimeout") |
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.
Keeping the core package zosmf agnostic makes sense.
Maybe we can move this response timeout somewhere in the zosmf package?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aaditya Sinha <75474786+aadityasinha-dotcom@users.noreply.github.com>
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.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! 😋
I do have one small request...
Doing this only in the zosmf package will impact only the functions dependent on the zosmf...
From the IBM documentation...
https://www.ibm.com/docs/en/zos/2.5.0?topic=services-zos-data-set-file-rest-interface#izuhpinfo_api_restfiles__title__5
I believe we would want to add this same funcitonality in the files.py.
And perhaps also add some validation around it (mostly to warn people that the value may be modified by z/OSMF
5 >= responseTimeout >= 600 😋
If possible, please keep in mind that more headers may need to be introduced in the future 🙏
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
|
Hi @aadityasinha-dotcom , just tagging for awareness: Seems that the build is failing with a lint error:
Can you please fix this once you get a chance? Thanks |
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.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.
Code changes LGTM! Please resolve failing unit tests (I've done this 😁) and add a changelog entry 😋
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
| async_threshold = profile.get("asyncThreshold") or profile.get("X-IBM-Async-Threshold") | ||
| if not async_threshold: |
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.
Although it's true that the X-IBM-Async-Threshold and X-IBM-Response-Timeout headers are mutually exclusive, Zowe team config files don't allow the asyncThreshold parameter to be defined in a z/OSMF profile, so I think this check can be removed.
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.
That's true, I also think asyncThreshold check can be removed.
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
| else: | ||
| if hasattr(self, "logger"): | ||
| self.logger.info("X-IBM-Async-Threshold present; X-IBM-Response-Timeout will be ignored by z/OSMF") |
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.
Please remove the else block - we don't need to log a warning when responseTimeout is missing on the profile. Everything else LGTM 😋
Signed-off-by: aadityasinha-dotcom <aadityasinha009@gmail.com>
t1m0thyj
left a comment
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 @aadityasinha-dotcom!
What It Does
Fixes #334
How to Test
Review Checklist
I certify that I have:
Additional Comments