Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

QuanMPhm
Copy link
Contributor

@QuanMPhm QuanMPhm commented Oct 8, 2024

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.

This is the first step of merging `openshift-acct-mgt` into the coldfront
plugin. The client code is yet used in any way, and will only serve to make
code review easier to follow.

The file will eventually be removed after merging of `openshift-acct-mgt` is complete.
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.

You have a new dependency and should update requirements.

@knikolla
Copy link
Collaborator

knikolla commented Oct 8, 2024

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
Copy link
Contributor Author

QuanMPhm commented Oct 8, 2024

@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
Copy link

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

@knikolla
Copy link
Collaborator

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

@QuanMPhm
Copy link
Contributor Author

@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
Copy link
Collaborator

@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.

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.

Copy over the code from openshift-acct-mgt with minimum changes
3 participants