reana-admin: fix setting storage quota for users after global default… #628
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #628 +/- ##
==========================================
+ Coverage 59.48% 59.50% +0.01%
==========================================
Files 32 32
Lines 3283 3289 +6
==========================================
+ Hits 1953 1957 +4
- Misses 1330 1332 +2
|
DaanRosendal
force-pushed
the
fix-setting-default-quota-limit
branch
3 times, most recently
from
92dee0a
to
b71fe57
Compare
reana_server/reana_admin/cli.py
Outdated
| emails=[user.email], | ||
| resource_name=resource.name, | ||
| resource_type=resource.type_.name, | ||
| limit=DEFAULT_QUOTA_LIMITS.get(resource.type_.name), |
There was a problem hiding this comment.
Works nicely, but perhaps we should check whether the setting is actually needed here, because otherwise some unnecessary operations would be called:
Quota limit 0 for 'cpu (processing time)' successfully set to users ['[email protected]']. │
Quota limit 0 for 'cpu (processing time)' successfully set to users ['[email protected]'].
This populates the table with zeros which may not be necessary. (Even though later upgrade to a real value would well.)
There was a problem hiding this comment.
Added a check so an update is only performed when a custom default limit has been set (default limit != 0). The table is populated by zeros anyway, but it's nice to keep the output clean and not perform any unnecessary actions like you said.
| check_interactive_sessions, | ||
| check_workflows, |
There was a problem hiding this comment.
We can sync in the team about using common import sorting technique across editors.
tests/test_cli.py
Outdated
| @@ -968,3 +970,33 @@ def test_check_workspaces( | |||
| if workflow: | |||
| assert len(result.errors) == 2 | |||
| assert any(workflow.workspace_path in str(error) for error in result.errors) | |||
|
|
|||
|
|
|||
| def test_quota_set_default_limits_with_user_with_custom_limits(default_user, session): | |||
There was a problem hiding this comment.
You can squash the test commit together with the feature commit. It's nice if features are coming with tests in the same atomic unit. Helps to make sure things work already from the start.
(And while squashing, you may want to restrict the commit log message line length.)
There was a problem hiding this comment.
Cosmetics: "_with_user_with_custom_limits" might be better said "for_users_with_custom_limits".
DaanRosendal
force-pushed
the
fix-setting-default-quota-limit
branch
5 times, most recently
from
47d735f
to
e7fd11d
Compare
This commit fixes a bug in the reana-admin CLI command ``quota-set-default-limits`` that prevented admins from retroactively setting a default quota limit for all existing users who do not have a custom quota limit set. Specifically, the bug was caused by the fact that the code assumed that all users who do not have a custom quota limit set have a quota limit of ``null``. This assumption is incorrect because when there is no default global quota limit set, each user's initial quota limit is set to zero, not ``null``. Note that any previously set user limits, either via old defaults or via custom settings, will be kept during the upgrade, and won't be automatically updated to match the new default limit value. Closes reanahub#625
DaanRosendal
force-pushed
the
fix-setting-default-quota-limit
branch
from
e7fd11d
to
7e84d56
Compare
Consideration: the code assumes a default quota limit of zero. This assumption can lead to unexpected behaviour for admins when they change their default quota limit from a previously defined value to a new one. This is because any previously set default limits will be treated as custom limits and thus won't be automatically updated to match the new default limit.
I tested several scenarios:
Single User Scenarios
Multiple Users Scenarios
Closes #625