Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

[merged] Refactor containers verb#792

Closed
baude wants to merge 1 commit intoprojectatomic:masterfrom
baude:refactor_containers
Closed

[merged] Refactor containers verb#792
baude wants to merge 1 commit intoprojectatomic:masterfrom
baude:refactor_containers

Conversation

@baude
Copy link
Member

@baude baude commented Dec 8, 2016

With the exception of fstrim, the containers verb has now been
refactored. It primarily now uses the containers object in its
implementation.

container.image_id = info['ImageID']
container.created = info['Created']
container.status = runtime['status']
container.status = container.state = runtime['status']
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have container.status and container.state?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on how the struct information is gather (i.e. inspect vs containers), they have State and/or Status and they differ. In ostree, we set both to be the same and handle it.

format(img, ', '.join([x.backend for x in img_in_backends])))

def get_backend_for_container(self, container, str_preferred_backend=None):
def get_backend_and_container(self, container, str_preferred_backend=None):
Copy link
Member

Choose a reason for hiding this comment

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

Confusing name of method. You are passing in "container" and returning con_obj. Either
container should be renamed to container_id, or function should be get_backend_and_container_object

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if con_obj:
container_in_backends.append((be, con_obj))
if len(container_in_backends) == 1:
return container_in_backends[0]
Copy link
Member

Choose a reason for hiding this comment

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

Is this multiple objects?

Copy link
Member Author

Choose a reason for hiding this comment

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

i dont understand your question

Copy link
Member

Choose a reason for hiding this comment

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

Function returns two objects. This seems to be returning 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it returns an instance of the backend and a container object seen there on line 89.


class Containers(Atomic):

FILTERALES= {"container": "id", "image": "image_name", "command": "command",
Copy link
Member

Choose a reason for hiding this comment

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

FILTERALES? Bad name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, although that was the previous name as well. Now called FILTER_KEYWORDS

@baude baude force-pushed the refactor_containers branch from ba759fd to 3b55727 Compare December 8, 2016 19:09
@rhatdan
Copy link
Member

rhatdan commented Dec 8, 2016

If tests pass, I will merge.

@baude baude force-pushed the refactor_containers branch 2 times, most recently from 6d566bd to 37131aa Compare December 8, 2016 20:00
With the exception of fstrim, the containers verb has now been
refactored.  It primarily now uses the containers object in its
implementation.
@baude baude force-pushed the refactor_containers branch from 37131aa to a9b235c Compare December 8, 2016 20:17
@baude
Copy link
Member Author

baude commented Dec 8, 2016

@rhatdan looks like i got it this time.

@rhatdan
Copy link
Member

rhatdan commented Dec 9, 2016

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit a9b235c has been approved by rhatdan

@rh-atomic-bot
Copy link

⌛ Testing commit a9b235c with merge e405c6a...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: rhatdan
Pushing e405c6a to master...

@rh-atomic-bot rh-atomic-bot changed the title Refactor containers verb [merged] Refactor containers verb Dec 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants