rest: add max inactivity period for interactive sessions to info endpoint #599
Conversation
giuseppe-steduto
force-pushed
the
add-inactivity-period-to-info
branch
from
28caa5b
to
140f23e
Compare
There was a problem hiding this comment.
It works nicely! I left couple of small comments how it could be potentially improved.
Can you please modify the PR/commit messages to use closes instead of addresses since the PR is not properly linked to the issue in the kanban board. I had to dig a bit to find it
| @@ -455,6 +455,17 @@ | |||
| }, | |||
| "type": "object" | |||
| }, | |||
| "maximum_interactive_session_inactivity_period": { | |||
There was a problem hiding this comment.
Since you modified the OpenAPI specification you have to call the generate_openapi_spec.py script to publish these changes to reana-commons (and create another PR there)
reana_server/rest/info.py
Outdated
| @@ -108,6 +108,7 @@ def info(user, **kwargs): # noqa | |||
| } | |||
| """ | |||
| try: | |||
| ui_config = REANAConfig.load("ui") | |||
There was a problem hiding this comment.
Have you considered injecting inactivity period value through Helm templates similarly as it is done for REANA_KUBERNETES_JOBS_MAX_USER_TIMEOUT_LIMIT?. Now it's a bit strange to get it from the UI configmap
There was a problem hiding this comment.
Right, it looks indeed like a way cleaner solution!
giuseppe-steduto
force-pushed
the
add-inactivity-period-to-info
branch
from
140f23e
to
cdd9b48
Compare
docs/openapi.json
Outdated
| "type": "string" | ||
| }, | ||
| "value": { | ||
| "type": "string" |
There was a problem hiding this comment.
Since this field could have a None value it should include "x-nullable": true flag similarly as for kubernetes_max_memory_limit field
Please not that after this change reana-client-go swagger specs will have to be regenerated
reana_server/config.py
Outdated
| "REANA_SESSION_MAX_INACTIVITY_PERIOD", "forever" | ||
| ) | ||
| if _reana_session_max_inactivity_period_env == "forever": | ||
| REANA_SESSION_MAX_INACTIVITY_PERIOD: Optional[str] = None |
There was a problem hiding this comment.
Small suggestion: perhaps it would be better to keep a long and descriptive name and use REANA_INTERACTIVE_SESSION prefix for this value so that it wouldn't be confused to other type of sessions reana might have? WDYT?
reana_server/rest/info.py
Outdated
| @@ -180,3 +185,4 @@ class InfoSchema(Schema): | |||
| maximum_workspace_retention_period = fields.Nested(StringNullableInfoValue) | |||
| default_kubernetes_jobs_timeout = fields.Nested(StringInfoValue) | |||
| maximum_kubernetes_jobs_timeout = fields.Nested(StringInfoValue) | |||
| maximum_interactive_session_inactivity_period = fields.Nested(StringInfoValue) | |||
There was a problem hiding this comment.
It should be StringNullableInfoValue because it might containt None value
giuseppe-steduto
force-pushed
the
add-inactivity-period-to-info
branch
from
cdd9b48
to
6b567d6
Compare
|
Works well, but this needs to be rebased! |
giuseppe-steduto
force-pushed
the
add-inactivity-period-to-info
branch
from
6b567d6
to
9becd8d
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #599 +/- ##
==========================================
+ Coverage 59.12% 59.15% +0.03%
==========================================
Files 32 32
Lines 3227 3232 +5
==========================================
+ Hits 1908 1912 +4
- Misses 1319 1320 +1
|
…oint Adds the `maximum_interactive_session_inactivity_period` value to the ones returned by `/api/info`. Closes reanahub/reana-client#657
giuseppe-steduto
force-pushed
the
add-inactivity-period-to-info
branch
from
9becd8d
to
c459270
Compare
Adds the
maximum_interactive_session_inactivity_periodvalue to the ones returned by/api/info.Addresses reanahub/reana-client#657