Skip to content

Conversation

@flaper87
Copy link
Contributor

@flaper87 flaper87 commented Aug 16, 2017

It's possible to interact with the Kubernetes/OpenShift API without auth
or even without a configuration file. This patch allows for creating an
ApiClient instance without loading it from a configuration file if it is
not set.

r? @chouseknecht

It's possible to interact with the Kubernetes/OpenShift API without auth
or even without a configuration file. This patch allows for creating an
ApiClient instance without loading it from a configuration file if it is
not set.
@flaper87 flaper87 force-pushed the master branch 2 times, most recently from b4c1dab to 13d5d86 Compare August 21, 2017 09:14
@flaper87
Copy link
Contributor Author

@fabianvf hey, do you happen to have any hint on why this is failing? Some of these failures seem to be unrelated but I might be wrong.


class KubernetesObjectHelper(BaseObjectHelper):
@staticmethod
def client_from_config(config_file, context):
Copy link
Member

Choose a reason for hiding this comment

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

It seems strange to me to put the logic for handling non-config based client creation in the client_from_config function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to put it here because the logic for client_from_config is in this library. Otherwise, I think I would have had to put the kubernetes specific logic in the kubernetes library and the rest somewhere in openshift-restclient-python. This would be inconsistent and I honestly don't think new_client_from_config would be a good place for it :(

I think it's fine to have this code here (based on my very limited knowledge of this codebase). Do you think there's a better place for it? Where would you put it? thnx

Copy link
Member

Choose a reason for hiding this comment

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

You're probably right, for now best to leave it here. Would you mind adding a TODO over this code branch to look at splitting the non-config logic out of client_from_config. Likely would be a larger refactor, I just want to be reminded whenever I grep the TODOs.

Copy link
Member

Choose a reason for hiding this comment

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

Just added them myself, thanks for the patch!

@fabianvf fabianvf merged commit 13d5d86 into openshift:master Aug 30, 2017
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.

2 participants