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

Metadata classes for image and container #204

Merged
merged 2 commits into from May 11, 2018

Conversation

rpitonak
Copy link
Collaborator

@rpitonak rpitonak commented May 10, 2018

Fixes #72

@@ -0,0 +1,48 @@
class Metadata:
Copy link
Member

Choose a reason for hiding this comment

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

I guess you want to inherit from object since we need conu to be Python 2 compatible.
See https://stackoverflow.com/questions/15374857/should-all-python-classes-extend-object#answer-15374884

Copy link
Member

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

This is definitely a good start! Once we are fine with the wording I propose to merge this and iterate on it with subsequent pull requests.

creation_timestamp=None, env_variables=None):
self.name = name
self.id = id
self.ports = ports
Copy link
Member

Choose a reason for hiding this comment

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

I would remove ports from this list since it has different meaning for container and images.

def __init__(self, name=None, id=None, ports=None, labels=None, command=None,
creation_timestamp=None, env_variables=None):
self.name = name
self.id = id
Copy link
Member

Choose a reason for hiding this comment

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

please use identifier since id is a builtin python function:

In [1]: help(id)
Help on built-in function id in module __builtin__:

id(...)
    id(object) -> integer
    
    Return the identity of an object.  This is guaranteed to be unique among
    simultaneously existing objects.  (Hint: it's the object's memory address.)


self.tags = tags
self.registry = registry
self.entrypoint = entrypoint
Copy link
Member

Choose a reason for hiding this comment

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

since entrypoint is docker-specific, I would remove it from the list and just live with command

name=name, id=id, ports=ports, labels=labels, command=command,
creation_timestamp=creation_timestamp, env_variables=env_variables)

self.tags = tags
Copy link
Member

Choose a reason for hiding this comment

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

could we replace tags and registry with image_names?

self.image = image
self.ipv4 = ipv4
self.ipv6 = ipv6
self.status = status
Copy link
Member

Choose a reason for hiding this comment

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

will this be enum? or string?

Copy link
Collaborator Author

@rpitonak rpitonak May 10, 2018

Choose a reason for hiding this comment

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

enum, I used the values that docker container support.

Copy link
Member

Choose a reason for hiding this comment

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

cool, sounds good!

creation_timestamp=creation_timestamp, env_variables=env_variables)
self.hostname = hostname
self.image = image
self.ipv4 = ipv4
Copy link
Member

Choose a reason for hiding this comment

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

we need to think a bit more about network addresses since one container can have more of them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ipv4 and ipv6 can be lists, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

yup, I would probably then name it ipv4_addresses

Copy link
Member

Choose a reason for hiding this comment

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

can be some ports associated only to some IPs? will we represent this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TomasTomecek ok I agree with name. @dhodovsk what about the dictionary
ipv4_adresses = {"10.0.0.1":8008, "10.0.02":8555} does it make sense?

@TomasTomecek
Copy link
Member

What about docs? I propose doing documentation in next pull request.

@TomasTomecek TomasTomecek merged commit 0dfcd30 into user-cont:master May 11, 2018
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.

None yet

4 participants