Run tests for all target types#185
Conversation
lachmanfrantisek
commented
Sep 18, 2018
lachmanfrantisek
commented
- Use coverage for tests.
- Run tests on all target types.
- Use name parameter for all targets.
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
TomasTomecek
left a comment
TomasTomecek
left a comment
There was a problem hiding this comment.
Overall, you upped the code quality greatly. Just a few questions: I request answers, not necessarily changes.
| self.insecure = insecure | ||
| self.image_name = ImageName.parse(target) | ||
| self.image_name_obj = ImageName.parse(target) | ||
| self.name = self.image_name_obj.name |
There was a problem hiding this comment.
These 3 lines are well done: it could really bite as badly if we changed a type of one of the attributes (happened to me recently actually in another project)
The only thing I don't like is the name "name": it's way too generic. If I didn't know the code, I would expect that self.name is the __repr__ name of the target class, not the target name itself.
We can leave it as it is, this is just a small observation.
There was a problem hiding this comment.
Any suggestions for the name name? I need something general matching also Dockerfiles...
There was a problem hiding this comment.
It is always like this:
def __init__(self, target, **_):
super().__init__()
self.name = target
What about target / source_name / target_source?
There was a problem hiding this comment.
target would indicate instance of Target.
source_name LGTM
target_source feels complicated to me.
or just target_name
|
|
||
| cmd = ["podman", "push", image_name, skopeo_target] | ||
| subprocess.check_call(cmd) | ||
|
|
| # "cmd_or_entrypoint": "PASS", | ||
| # "no_root": "FAIL", | ||
| } | ||
|
|
There was a problem hiding this comment.
what about the cmd_or and no_root checks? will they work with podman?
There was a problem hiding this comment.
no_rootwill work without changescmd_or_entrypointwill need some small changes (The config dictionary has a slightly different format.)
I can add them in the other pull request.
|
Let me know which changes you intend to make and I'll merge once all is cool. |
|
@TomasTomecek I'll change the |
Signed-off-by: lachmanfrantisek <flachman@redhat.com>
|
@TomasTomecek I am done for now... |
|
Nicely done, let's unleash the tests! |
2 participants
