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

Drop entity classes #292

Merged
merged 2 commits into from
Jan 31, 2018
Merged

Drop entity classes #292

merged 2 commits into from
Jan 31, 2018

Conversation

moolitayer
Copy link
Collaborator

@moolitayer moolitayer commented Jan 23, 2018

Fix #198

Drop entity classes (e.g Kubeclient::Service).
Using them required dynamically creating classes under Kubeclient namespace during discovery.
We have had several hairy bugs around this area, and the code does not have real benefit for users.

Part of: #199

@moolitayer moolitayer added the wip label Jan 23, 2018
@moolitayer moolitayer removed the wip label Jan 24, 2018
@moolitayer moolitayer changed the title [WIP] Drop entity classes Drop entity classes Jan 24, 2018
@moolitayer
Copy link
Collaborator Author

@cben @grosser @kirs @jimmidyson Please take a look

@moolitayer
Copy link
Collaborator Author

@jhernand please take a look

@jhernand
Copy link
Contributor

@moolitayer will this be backported to 2.x, or is it only for 3.x?

README.md Outdated
Checking the type of a resource can be done using:
```
> pod.kind
Pod
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a string — quotes missing?

@moolitayer moolitayer force-pushed the drop_entity_classes branch 2 times, most recently from d317c2e to f3d9841 Compare January 28, 2018 16:48
@@ -113,7 +113,7 @@ def delete_replication_controllers(client, namespace, replication_controllers)
private

def construct_base_rc(namespace)
rc = Kubeclient::ReplicationController.new
rc = Kubeclient::Resource.new
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't one set rc.kind now that it's not implied by class?
IIUC, it works because we use these objects via create_replication_controller.
It's not ideal end state (though maybe fine for now) if some Resource objects around will have .kind and some won't, and what works depends on how you use it...
cf #235, #241.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is correct though it did not change in this PR.

@@ -271,7 +252,7 @@ def watch_entities(resource_name, options = {})
#
# Default response type will return a collection RecursiveOpenStruct
# (:ros) objects, unless `:as` is passed with `:raw`.
def get_entities(entity_type, klass, resource_name, options = {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are get_entities, get_entity public APIs? 👍 on this change, but if yes it should be documented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are not, we agreed to add a comment separately in the README.md saying any usage
not documented in the README.md is not supported.

@cben cben merged commit e8bcf76 into ManageIQ:master Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants