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

[merged] Refactor images#771

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

[merged] Refactor images#771
baude wants to merge 1 commit intoprojectatomic:masterfrom
baude:images_refactor

Conversation

@baude
Copy link
Member

@baude baude commented Nov 28, 2016

Covers all but verify and generate. This is a refactoring of the
images subverbs (i.e. info, version, delete, ...)

@baude
Copy link
Member Author

baude commented Nov 28, 2016

@giuseppe can you review this as well? Also, there seems to be a system_containers test failure on my system; if it shows here, can you help me understand what is going wrong?

@giuseppe
Copy link
Collaborator

I think the test is failing because we are now calling the backend "ostree", while we used to refer to the type of image as "system" (or "user" for user mode).

Could you try to s|type=system|type=ostree| in the bash/atomic and tests/integration/test_system_containers.sh files?

Copy link
Collaborator

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

a few inline comments

Atomic/delete.py Outdated
results = 2

return results
# def _delete_local(self, targets, force=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this code is not needed anymore, let's just drop it

Copy link
Member

Choose a reason for hiding this comment

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

I agree drop it

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, was holding it to be certain it passed tests first.

Atomic/delete.py Outdated
else:
results = self._delete_local(self.args.delete_targets, self.args.force)
return results
# Peform the delete
Copy link
Collaborator

Choose a reason for hiding this comment

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

s|Peform|Perform|

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

Atomic/info.py Outdated
buf.write("Image Name: {}\n".format(info_name))
buf.writelines(sorted(["{}: {}\n".format(k, v) for k,v in list(img_obj.labels.items())]))
if img_obj.template_variables_set:
buf.write("\n\nEnvironment variables with default value, but overridable with --set:\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use "Template variables", as they are not passed as env variables for system containers.

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

Atomic/info.py Outdated
buf.write("\n\nEnvironment variables with default value, but overridable with --set:\n")
buf.writelines(["{}: {}\n".format(k, v) for k,v in list(img_obj.template_variables_set.items())])
if img_obj.template_variables_unset:
buf.write("\n\nEnvironment variables that has no default value, and must be set with --set:\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

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

s = round(size/p, 2) # pylint: disable=round-builtin,old-division
if s > 0:
return '%s %s' % (s, size_name[i])
return '0B' No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing '\n' here

Copy link
Member Author

Choose a reason for hiding this comment

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

Added newline, thanks

Atomic/delete.py Outdated
results = 2

return results
# def _delete_local(self, targets, force=False):
Copy link
Member

Choose a reason for hiding this comment

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

I agree drop it

Atomic/images.py Outdated
class Images(Atomic):
def __init__(self):
super(Images, self).__init__()
self.beu = backendutils.BackendUtils()
Copy link
Member

Choose a reason for hiding this comment

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

Not crazy about self.beu. Means nothing to me.

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

@baude baude force-pushed the images_refactor branch 5 times, most recently from 4d7eb72 to 1a589db Compare November 29, 2016 20:22
@baude
Copy link
Member Author

baude commented Nov 29, 2016

@rhatdan addressed feedback, etc

Atomic/atomic.py Outdated
self.syscontainers.set_args(self.args)
try:
self.syscontainers.set_args(self.args)
except (NameError, AttributeError):
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 dangerous, since some args might not have been set?

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2016

Other then question LGTM. If it is good with @giuseppe Merge it.

Covers all but verify and generate.  This is a refactoring of the
images subverbs (i.e. info, version, delete, ...)

Added in a unittest for list and info.
@baude
Copy link
Member Author

baude commented Nov 29, 2016

I'll revert that bit and see if it passes tests. @giuseppe reviewed it this morning. If it passes, Ill merge.

@baude
Copy link
Member Author

baude commented Nov 29, 2016

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 4b43643 has been approved by baude

@rh-atomic-bot
Copy link

⌛ Testing commit 4b43643 with merge ef984ed...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: baude
Pushing ef984ed to master...

@rh-atomic-bot rh-atomic-bot changed the title Refactor images [merged] Refactor images Nov 29, 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.

4 participants