Refactor to allow resource-agnostic validation#248
Refactor to allow resource-agnostic validation#248QuanMPhm merged 1 commit intonerc-project:mainfrom
Conversation
knikolla
left a comment
There was a problem hiding this comment.
This is a good start, but not going as far as I would like in terms of making it "resource-agnostic", as you still have a lot of if resource_name in OPENSHIFT|OPENSTACK.
Try thinking about a way to push those resource-specific functions inside the respective allocators and creating a new function in the base Allocator that serves as the abstraction.
See my comment about validate project exist.
src/coldfront_plugin_cloud/management/commands/validate_allocations.py
Outdated
Show resolved
Hide resolved
|
Note-to-self: The branch name for the original solution (before time of this comment) is |
9153d7b to
a86da68
Compare
40883e2 to
129234b
Compare
|
@knikolla I've cleaned house on |
|
@QuanMPhm can you resolve conflicts (and update to new quota system if necessary). I would like to have this be part of next month's update. |
129234b to
a0ba80d
Compare
|
@knikolla I have resolved all conflicts. This is ready to review |
|
I did a quick first pass, I like this approach much more. Will do a second pass early tomorrow morning. |
a0ba80d to
a22bf80
Compare
There was a problem hiding this comment.
Pull request overview
Refactors validate_allocations to be more resource-agnostic by delegating validation/sync behavior to each resource allocator (OpenStack/OpenShift/ESI), aligning behavior across resource types as requested in #239 (built on #245).
Changes:
- Move common validation/sync logic into
ResourceAllocator(user sync + quota comparison/apply helper) and implement per-resourceset_project_configuration()flows. - Update OpenStack/OpenShift allocators to own quota validation/application (and OpenShift labels/limitrange validation).
- Update functional/unit tests to use the new public
get_project()API and to cover “new quota attribute added after allocations exist”.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coldfront_plugin_cloud/tests/unit/openshift/test_project.py | Switch to get_project() public API. |
| src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py | Update logging assertion target; add functional test for new quota attribute behavior. |
| src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py | Switch to get_project() public API in functional checks. |
| src/coldfront_plugin_cloud/openstack.py | Implement set_project_configuration() + quota validation/apply; adjust Swift quota handling. |
| src/coldfront_plugin_cloud/openshift.py | Implement set_project_configuration() + limitrange/label/quota validation/apply; expose get_project(). |
| src/coldfront_plugin_cloud/management/commands/validate_allocations.py | Simplify command to iterate resource types and call allocator-provided configuration sync. |
| src/coldfront_plugin_cloud/base.py | Add shared user sync + quota comparison/apply helpers and allocation_str. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/coldfront_plugin_cloud/management/commands/validate_allocations.py
Outdated
Show resolved
Hide resolved
src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py
Outdated
Show resolved
Hide resolved
a22bf80 to
6ebd705
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
knikolla
left a comment
There was a problem hiding this comment.
This looks good to me, the only outstanding things is the failed_validation comment by Copilot.
With regards to the Swift comments, if you want to do a bit more investigating and see if they are important to fix.
|
@naved001 would appreciate a pass from you. |
Quota validation will now behave the same for both resource types. OpenStack's particular use of default quotas is reflected in a new test in `openstack/test_allocation.py` OpenStack integration code is slightly changed to better handle missing object storage quotas Much of the validation logic has been pushed into `base.py`, `openshift.py`, and `openstack.py`
6ebd705 to
30236f0
Compare
Closes #239. This PR consists of the last commit. Built on top of #245. More details in the commit message.
@knikolla @jtriley I have a small bug in how error handling is done when the object storage quota is missing. Just want to confirm that my change is desirable.