WIP: Check for inherited optional labels #213
Conversation
This is needed for checks that require both the image and the Dockerfile it was built from. Signed-off-by: Tim Waugh <[email protected]>
Signed-off-by: Tim Waugh <[email protected]>
|
Can one of the admins verify this patch? |
|
I've started to work on this some time ago in #152 (on 21 Jun 2018) You can take some inspiration from it if you wish, but I never finalized that. I am not sure if we need a new target, but it can maybe solve my problems with getting the info about the base image when we only have an image. |
|
[test] |
| @@ -50,6 +50,9 @@ def cli(): | |||
| @click.command(name="check", | |||
| context_settings=CONTEXT_SETTINGS) | |||
| @click.argument('target', type=click.STRING) | |||
| @click.option('--supplementary-target', type=click.STRING, | |||
There was a problem hiding this comment.
Internally the input for colin is an image. I wonder how do we discover the specific dockerfile.
There was a problem hiding this comment.
Oh, is colin not shown the Dockerfile used to build the image?
I wonder if this check is a good fit for colin at all. I saw the 'dockerfile' target and assumed it was something it was also told.
There was a problem hiding this comment.
Internally, colin testing is initiated when an osbs build completes successfully. The input is only the name of the image (within the registry), colin then fetches it to ostree repo and starts the analysis.
I think it's feasiable to also link to the specific dockerfile which was used to produce the image -- we can still get the dockerfile from inside of the image, right?
There was a problem hiding this comment.
Only for OSBS-built images. Is that OK?
There was a problem hiding this comment.
Yes, unless @lachmanfrantisek objects. We can easily implement that check that it would only apply to OSBS' built images, otherwise it would fail with some reasonable error message.
There was a problem hiding this comment.
The absolutely ideal input would be the output of skopeo inspect $fq_name_parent_image. Then I can just look at .Labels and compare directly.
There was a problem hiding this comment.
I am fine with either way and from unit test POV it might be simpler to have output of skopeo inspect But on the other hand could hide network operations.
Choose whatever you/Tomas like more. And I'll provide it in CVP :-)
There was a problem hiding this comment.
I'm also fine with either way; I'm just slightly concerned with passing json via CLI or env vars, that's it. But since openshift does the same thing and apparently it works fine, let's pass labels directly.
There was a problem hiding this comment.
Hi, I am not sure how general we want this new check to be.
So we can either pass directly inspect json from parent OR
we can basically pass secondary_target (optional), maybe even secondary_target_type (or we can use same target_type for secondary_target)
that way we can use Target class also for parent target
and then we will just simply use those 2 targets for that inherited optional labels check
what do you think ? @TomasTomecek @lachmanfrantisek @lslebodn @twaugh
There was a problem hiding this comment.
just realized that, when we use target_type dockerfile, this check doesn't make sense, so in that case we would either ignore that secondary_target or forbid it with dockerfile type
|
I am taking over from @twaugh please check my PR: @TomasTomecek @lachmanfrantisek @lslebodn @twaugh for 2nd target for parent will be used same target-type as regular target |
|
closing this one then |
I'm opening this pull request even though it's not finished, for guidance about whether this is the right direction and whether colin is a good fit for this check.
The idea is that any container image which inherits an optional label (such as 'description') should also declare its own value for that label. If it doesn't, those label values will refer to the parent image and so will be inaccurate.
To do this the check needs simultaneous access to both the container image and the Dockerfile which was used to build it. For this I've added a new target and check type, "image+dockerfile".
Please advise on whether this seems like the right approach.
Thanks!