Skip to content

Conversation

QuanMPhm
Copy link
Contributor

@QuanMPhm QuanMPhm commented Apr 3, 2025

Closes #200. Built on top of #201. This PR consists of the last commit. 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.

@QuanMPhm QuanMPhm requested a review from hakasapl April 23, 2025 18:40
Copy link
Contributor

@naved001 naved001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a good feature to have. Just a question inline

@QuanMPhm QuanMPhm force-pushed the feature/add_attr branch 2 times, most recently from 2cdabf5 to 1f7348f Compare May 6, 2025 20:58
@QuanMPhm QuanMPhm requested a review from naved001 May 7, 2025 17:09
@QuanMPhm
Copy link
Contributor Author

@naved001 @knikolla This has been rebased and updated, and now has priority because the IBM Storage PR will now be built on it.

…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.
Copy link
Collaborator

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Some comments that don't need to be fixed now:

  • I don't love that the behavior of validate_allocations between OpenStack and OpenShift is now different. We should try to factor out the commonalities and implement features like this in an agnostic way.
  • Could the else have been replaced with an elif to keep the depth of the conditional statements flatter?

@knikolla knikolla merged commit e79cc36 into nerc-project:main Aug 27, 2025
4 checks passed
@QuanMPhm
Copy link
Contributor Author

Could the else have been replaced with an elif to keep the depth of the conditional statements flatter?

@knikolla Hilariously enough, @naved001 has already asked a similar question before. I'll try to refactor this code block within this year. I could use elif, but it would lead to some repeated code :P

@knikolla
Copy link
Collaborator

Could the else have been replaced with an elif to keep the depth of the conditional statements flatter?

@knikolla Hilariously enough, @naved001 has already asked a similar question before. I'll try to refactor this code block within this year. I could use elif, but it would lead to some repeated code :P

Ah, makes complete sense.

I had a similar moment of understanding before I left on vacation, which I clearly forgot when coming back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the feature to add new quota attributes to pre-existing allocations
3 participants