-
Notifications
You must be signed in to change notification settings - Fork 71
Conversation
msg = "%s %s" % (status_code, return_data) | ||
logger.error(msg) | ||
raise ProviderFailedException(msg) | ||
return self.oc.process_template(url, template) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is OpenShiftProvider._process_template
still needed?
I would probably get rid of if this function (_process_template
) now. It doesn't do anything useful, everything important is in OpenshiftClient.process_tempate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did not remove _process_template
is because of the dependency on get_url
. Also, I see no reason to move get_url
to OpenshiftClient
.
- Move interaction with remote API to fetch resources to different methods, for easy mocking.
…enShiftProvider to OpenshiftClient. This will help us to mock out the external world dependencies and test the core logic in OpenShiftProvider.
63b496e
to
b58c279
Compare
to remove external dependencies from core logic. Now, _process_artifacts() reads artifact files in a loop and calls _process_artifact_data() with the parsed artifact data to process it.
still WIP or done now? |
I have covered most of our core logic with the unittests, except for the external dependencies. Test coverage:
|
self.openshift_api = openshift_api | ||
self.kubernetes_api = kubernetes_api | ||
|
||
def get_oapi_resources(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably make this function generic:
get_api_resources(self, type) where type = 'kubernetes' or 'openshift'
don't have to change it, just sayin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
and refactored _parse_kubeconf() to move out external dependency (file access) from core logic.
b7d9b2d
to
789bd89
Compare
789bd89
to
ae16441
Compare
LGTM. @kadel can you +1 and we can merge this |
👍 LGTM |
Tests and related refactor for openshift provider using Openshift API.