Implemented feature to add new attributes to existing Openshift allocations #202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
naved001
reviewed
May 6, 2025
| expected_value = self.set_default_quota_on_allocation(allocation, allocator, attr) | ||
| logger.warning(f"Added default quota for {attr} to allocation {allocation_str} to {expected_value}") | ||
|
|
||
| if not (current_value == expected_value): |
There was a problem hiding this comment.
why did you have to make it an if inside of an else and move it down? why couldn't we have done this?
elif not (current_value == expected_value):
# do the thing for this condition
elif (current_value is None and expected_value is None):
# do the new thing of setting default quotaThere was a problem hiding this comment.
Funny enough. It seems you already asked something similar to this before.
I take it that this means these if statements are very confusing, and I've added the comment block below. @naved001 Should I make an issue to refactor this in the future?
# We just checked the case where the quota value is set in the cluster
# but not in coldfront. This is the only case the cluster value is the
# "source of truth" for the quota value
# If the coldfront value is set, it is always the source of truth.
# But first, we need to check if the quota value is set anywhere at all.
# TODO (Quan): Refactor these if statements so that we can remove this comment block?
There was a problem hiding this comment.
@QuanMPhm LMAO. yes, please add a brief comment to explain this otherwise I will, without fail, ask the same question again in a few months.
when determining what attributes an allocation should have This is done by a function `get_expected_attributes` in `tasks.py` to fetch the list of expected attributes
…ations `validate_allocations` will now check if an Openshift allocation does not have a quota value set on either the Coldfront or Openshift side. In this case, it will set the default quota value for the allocation. Due to the complexity of Openstack quotas, this feature is only implemented for Openshift allocations for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #200. Built on top of #201. This PR consists of the last commit.
validate_allocationswill now check if an Openshift allocation does not have a quota value set on either the Coldfront or Openshift side. In this case, it will set the default quota value for the allocation.Due to the complexity of Openstack quotas, this feature is only
implemented for Openshift allocations for now.