search

nerc-project / coldfront-plugin-cloud

ColdFront plugin to manage resource allocations on OpenStack and OpenShift
GNU General Public License v3.0
4 stars 10 forks source link

Copied over Openshift client code from `openshift-acct-mgt` #176

Closed QuanMPhm closed 2 weeks ago

QuanMPhm commented 1 month ago

Closes #137. This is the first step of merging openshift-acct-mgt into the coldfront plugin. The client code in moc_openshift.py is not used in any way, and will only serve to make code review easier to follow. My plan is that as I modify each function in coldfront_plugin_cloud/openshift.py to call the Openshift API directly, I will move over some code from moc_openshift.py.

The file will eventually be removed after merging of openshift-acct-mgt is complete.

knikolla commented 1 month ago

Why do you think adding the entire moc_openshift module will make things easier to review rather than just implementing functions one by one?

QuanMPhm commented 1 month ago

@knikolla My reasoning was: As I implement each function in coldfront_plugin_cloud/openshift.py, I will likely be copying some code from moc_openshift.py. My intention was that I would remove code from moc_openshift that I copied, making it clearer in the diff which lines were copied over, and how they originally looked like in case these lines were modified as I merge them into coldfront_plugin_cloud/openshift.py.

Typing it out fully like this, I think it seems a bit. tacky. I'll follow your advice.

joachimweyl commented 1 month ago

@knikolla what needs to be done to approve this PR?

knikolla commented 1 month ago

@joachimweyl Quan's last response seems to imply that he will rework the PR to incorporate my feedback.

QuanMPhm commented 1 month ago

@knikolla I should have realized my comment to you sounded more ambiguous than I thought. Since it doesn't make sense to copy over the moc_openshift module, I don't believe it would make sense to have an entire PR dedicated to copying over code. Since the approach of this merging process will be to implement each function in openshift.py one-by-one, I will close this PR unless you believe there's certain code that should be copied.

knikolla commented 1 month ago

@knikolla I should have realized my comment to you sounded more ambiguous than I thought. Since it doesn't make sense to copy over the moc_openshift module, I don't believe it would make sense to have an entire PR dedicated to copying over code. Since the approach of this merging process will be to implement each function in openshift.py one-by-one, I will close this PR unless you believe there's certain code that should be copied.

@knikolla you can "copy" code one function at a time if you find it useful. There's certainly stuff that's usable from moc_openshift. It just may not need to be copied all that once, or without modifications.

QuanMPhm commented 2 weeks ago

Closing this as it has now been replaced by other merging PRs (#175 )