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

[1LP][RFR] replacing old openshift api with new one #255

Merged
merged 2 commits into from
Jun 8, 2018

Conversation

izapolsk
Copy link
Contributor

No description provided.

if entity not in entities:
entities.append(entity)
return entities
namespace = namespace if namespace else self.default_namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user does not specify a namespace, why lock it in to default? They should specify default if they want routes from the default namespace


def list_image_streams(self, namespace=None):
"""Returns list of image streams"""
namespace = namespace if namespace else self.default_namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

entity = Project(self, meta['name'])
entities.append(entity)
return entities
return self.o_api.list_project().items

def list_template(self, namespace=None):
"""Returns list of templates"""
namespace = namespace if namespace else self.default_namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above. Don't lock into default namespace

return getattr(cls, 'KIND', None) or cls.__name__

@classmethod
def create(cls, provider, payload):
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see any replacement for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all this stuff is available thru o_api. I can add missing create_ or patch_ methods if those are really used somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not used at the moment but I would like to start updating our tests to use them. Right now, we grab an object and perform various actions on it. I would like to change that to: create an object, perform actions, remove object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it won't be difficult to add missing methods. I'll help you.
Let me know when you decide to do that. I think we will restructure module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I agree with this.

@izapolsk izapolsk force-pushed the removal_old_ocp_methods branch 2 times, most recently from 7e62daf to 627d725 Compare June 8, 2018 16:11
@izapolsk izapolsk changed the title [WIP] replacing old openshift api with new one [RFR] replacing old openshift api with new one Jun 8, 2018
@izapolsk izapolsk requested review from mshriver and quarckster June 8, 2018 16:12
@izapolsk izapolsk force-pushed the removal_old_ocp_methods branch from 627d725 to b1a2d42 Compare June 8, 2018 16:25
@dajoRH dajoRH removed the WIP label Jun 8, 2018
@izapolsk izapolsk requested review from bsquizz and removed request for quarckster June 8, 2018 16:31
Copy link
Contributor

@bsquizz bsquizz 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, I found 1 thing to nitpick you on :P

It's unfortunate that we can't pass namespace=None directly into all these list method calls, I looked at the code and saw they won't allow namespace to be None so I see why you did this ;)

"""
pods = self.list_pods(namespace=namespace)
containers = []
containers.extend([pod.spec.containers for pod in pods])
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of extending the empty list, can we just do this?
containers = [pod.spec.containers for pod in pods]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was more code before

aggregate_cpu, aggregate_mem = 0, 0
for node in self.list_node():
aggregate_cpu += int(node.status.capacity['cpu'])
aggregate_mem += int(round(int(node.status.capacity['memory'][:-2]) * 0.00000102400))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have just a small comment here --
# converting KiB to GB. 1KiB = 1.024E-6 GB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bsquizz bsquizz changed the title [RFR] replacing old openshift api with new one [WIP] replacing old openshift api with new one Jun 8, 2018
@dajoRH dajoRH added the WIP label Jun 8, 2018
… wrapanapi->ocp is switched to dynamic client
@izapolsk izapolsk force-pushed the removal_old_ocp_methods branch from b1a2d42 to cc3134a Compare June 8, 2018 19:19
@izapolsk izapolsk changed the title [WIP] replacing old openshift api with new one [RFR] replacing old openshift api with new one Jun 8, 2018
@izapolsk izapolsk requested a review from bsquizz June 8, 2018 19:21
@dajoRH dajoRH removed the WIP label Jun 8, 2018
@bsquizz bsquizz changed the title [RFR] replacing old openshift api with new one [1LP][RFR] replacing old openshift api with new one Jun 8, 2018
@mshriver
Copy link
Collaborator

mshriver commented Jun 8, 2018

@jawatts is working on a change to ManageIQ/integration_tests to get rid of the Volume class import.

@mshriver mshriver merged commit 3305d8e into RedHatQE:master Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants