[Fix/enhancement] dockerimage labels check and attempted to deduplicate #309

jstourac · 2023-11-03T23:07:39Z

Relates to: #292

Docker image labels reviewed

Description

This change tries to deduplicate some strings in our docker image labels with ability to propagate some common values directly from the Makefile so we don't need to change these values with every release branch or for every newly introduced docker image.

There were some minor fixes in the labels introduced too in this commit.

Notable points for discussion:

I used value of the RELEASE variable for the build-ref at the moment - I understand that this may not be ideal. We may introduce separate value for this in Makefile or we can try to compute the value (either ref hash or branch name) dynamically in Makefile too. But in general - the value of the RELEASE should match what we've used so far.
Also, some labels (authoritative-source-url and build commit ref at least) could be propagated directly from Makefile - in this case I mean by using the --label parameter. There are two drawbacks I can think of:
1. We could lost them in case of an individual build because this way these won't be enforced by Dockerfile itself
2. In this Dockerfile we have a slightly different naming for some labels which includes s2i infix - we would have to assure we don't change it wrongly.
  - Actually - do we want these s2i infixes?
I didn't touch the Anaconda Base Dockerfile - looks like the labels there should deserve review too.

How Has This Been Tested?

I did a brief build of one image so far to check the idea behind the changes works. Will do more once the conversation here is finalized:

make codeserver-ubi9-python-3.9 -e IMAGE_REGISTRY=quay.io/jstourac/workbench-images

Result images can be seen in https://quay.io/repository/jstourac/workbench-images?tab=tags&tag=latest

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2023-11-03T23:08:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vaishnavihire for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

base/c9s-python-3.9/Dockerfile

atheo89 · 2023-11-10T15:28:14Z

Hey Jan! This changes looks good to me 🙂 I added a small comment on the proposal!

jstourac · 2023-12-07T01:18:43Z

Okay, I rebased and updated with the change so that the AUTHORITATIVE_SOURCE_URL is now propagated also from Makefile. And I also used the BUILD_COMMIT_REF value in the io.openshift.build.source-location value.

jstourac · 2023-12-08T17:12:15Z

Regarding the Anaconda - there are two LABEL definitions in the Dockerfile:

https://github.com/opendatahub-io/notebooks/blob/main/base/anaconda-python-3.8/Dockerfile#L25C1-L34C48

LABEL summary="$SUMMARY" \
      description="$DESCRIPTION" \
      io.k8s.description="$DESCRIPTION" \
      io.k8s.display-name="Anaconda Python 3.8" \
      io.openshift.expose-services="8080:http" \
      io.openshift.tags="anaconda,builder,python,python38,python-38,anaconda-python38" \
      name="anaconda-python38" \
      version="1" \
      usage="s2i build https://github.com/sclorg/s2i-python-container.git --context-dir=3.8/test/setup-test-app/ ubi8/python-38 python-sample-app" \
      maintainer="[email protected] for now..."

and

https://github.com/opendatahub-io/notebooks/blob/main/base/anaconda-python-3.8/Dockerfile#L87C1-L91C98

LABEL name="odh-notebook-base-ubi8-anaconda-python-3.8" \
      summary="Anaconda based Python 3.8 base image for ODH notebooks" \
      description="Anaconda Base Python 3.8 builder image based on UBI8 for ODH notebooks" \
      io.k8s.display-name="Anaconda Python 3.8 base image for ODH notebooks" \
      io.k8s.description="Anaconda Base Python 3.8 builder image based on UBI8 for ODH notebooks"

Is there anything what we want to do with it? Do we want to update the second section to similar format as is e.g. in:

https://github.com/opendatahub-io/notebooks/blob/main/base/ubi9-python-3.9/Dockerfile#L3C1-L11C91

LABEL name="odh-notebook-base-ubi9-python-3.9" \
      summary="Python 3.9 base image for ODH notebooks" \
      description="Base Python 3.9 builder image based on UBI9 for ODH notebooks" \
      io.k8s.display-name="Python 3.9 base image for ODH notebooks" \
      io.k8s.description="Base Python 3.9 builder image based on UBI9 for ODH notebooks" \
      authoritative-source-url="https://github.com/opendatahub-io/notebooks" \
      io.openshift.build.commit.ref="main" \
      io.openshift.build.source-location="https://github.com/opendatahub-io/notebooks/tree/main/base/ubi9-python-3.9" \
      io.openshift.build.image="quay.io/opendatahub/workbench-images:base-ubi9-python-3.9"

harshad16

initial thought which made me hesitate were that, we would need to do this separately in downstream, however as we pretty much follow the similar methodology.
This would indeed help with both streams.

@jstourac can you rebase and update the PR.

harshad16 · 2024-01-29T22:15:43Z

Regarding the Anaconda - there are two LABEL definitions in the Dockerfile:
...
Is there anything what we want to do with it? Do we want to update the second section to similar format as is e.g. in:

https://github.com/opendatahub-io/notebooks/blob/main/base/ubi9-python-3.9/Dockerfile#L3C1-L11C91

LABEL name="odh-notebook-base-ubi9-python-3.9" \
      summary="Python 3.9 base image for ODH notebooks" \
      description="Base Python 3.9 builder image based on UBI9 for ODH notebooks" \
      io.k8s.display-name="Python 3.9 base image for ODH notebooks" \
      io.k8s.description="Base Python 3.9 builder image based on UBI9 for ODH notebooks" \
      authoritative-source-url="https://github.com/opendatahub-io/notebooks" \
      io.openshift.build.commit.ref="main" \
      io.openshift.build.source-location="https://github.com/opendatahub-io/notebooks/tree/main/base/ubi9-python-3.9" \
      io.openshift.build.image="quay.io/opendatahub/workbench-images:base-ubi9-python-3.9"

Updating the 2nd part exclusively the jupyter/anaconda-x part , same as other would be prefered.

jstourac · 2024-01-30T07:11:48Z

Thank you, @harshad16, for your review. I'll try to find some time for this this week 🙂

jstourac · 2024-02-02T18:36:58Z

base/anaconda-python-3.8/Dockerfile

+      authoritative-source-url="${AUTHORITATIVE_SOURCE_URL}" \
+      io.openshift.build.commit.ref="${BUILD_COMMIT_REF}" \
+      io.openshift.build.source-location="${AUTHORITATIVE_SOURCE_URL}/tree/${BUILD_COMMIT_REF}/base/anaconda-python-3.8" \
+      io.openshift.build.image="${IMAGE_REGISTRY}:base-c9s-python-3.9" TODO ?????


@harshad16 I'm not quite sure what value should be put on this place. I wasn't able to find the image in the https://quay.io/repository/opendatahub/workbench-images - did I miss it somehow? 🤔

This particular is not built on the ODH,
it is exclusively only built downstream.
we should probably clean a few of these.

I see. So, do you prefer me to remove just this io.openshift.build.image label or all four in this case?

The first three are still valid and could be reused for the downstream part. Although, if there is no image build in upstream, I understand that it doesn't make much sense to have them here either.

yeah true,
we can remove all of these:

authoritative-source-url io.openshift.build.commit.ref io.openshift.build.source-location io.openshift.build.image

as this more for anyone , who would like to build it, though we wouldn't be building it in upstream.

Thank you, Harshad! Updated 🙂

This change tries to deduplicate some strings in our docker image labels with ability to propagate some common values directly from the Makefile so we don't need to change these values with every release branch or for every newly introduced docker image. There were some minor fixes in the labels introduced too in this commit. --- Relates to: opendatahub-io#292

harshad16 · 2024-02-19T20:30:52Z

The PR looks great, my major concern is this would break the current CI build from openshift CI
and that doesn't completely rely on makefile.
https://github.com/openshift/release/blob/358594a0d50341051cdd9eb1137757a734fab063/ci-operator/config/opendatahub-io/notebooks/opendatahub-io-notebooks-main.yaml#L8
the CI , doesn't take in build args, which make cause this issue.
Result image of this PR needs to be tested.

jstourac · 2024-02-23T23:20:37Z

Thank you, Harshad, for pointing this out. I missed that bits. I'll try to check whether I am able to amend our CI so we can merge this eventually.

openshift-merge-robot · 2024-03-15T20:38:32Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

harshad16 · 2024-05-31T03:49:05Z

The PR looks great, my major concern is this would break the current CI build from openshift CI and that doesn't completely rely on makefile. https://github.com/openshift/release/blob/358594a0d50341051cdd9eb1137757a734fab063/ci-operator/config/opendatahub-io/notebooks/opendatahub-io-notebooks-main.yaml#L8 the CI , doesn't take in build args, which make cause this issue. Result image of this PR needs to be tested.

Due to this, we are changing this PR to the draft.

openshift-ci · 2024-05-31T04:40:43Z

@jstourac: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from atheo89 and dibryant November 3, 2023 23:07

jstourac self-assigned this Nov 3, 2023

jstourac force-pushed the enhanceDockerimageLabels branch 2 times, most recently from ee03be3 to eee5072 Compare November 6, 2023 20:05

jstourac requested a review from harshad16 November 6, 2023 20:08

This was referenced Nov 6, 2023

RHOAI: Build VSCode notebook on RHEL9 base red-hat-data-services/notebooks#94

Closed

Add a GitHub action to perform validation of params.env file jstourac/notebooks#2

Merged

atheo89 reviewed Nov 10, 2023

View reviewed changes

base/c9s-python-3.9/Dockerfile Outdated Show resolved Hide resolved

jstourac force-pushed the enhanceDockerimageLabels branch from eee5072 to 650db9b Compare December 7, 2023 01:15

harshad16 reviewed Jan 29, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Jan 29, 2024

jstourac force-pushed the enhanceDockerimageLabels branch from 650db9b to 32e90ab Compare February 2, 2024 18:33

openshift-merge-robot removed the needs-rebase label Feb 2, 2024

jstourac commented Feb 2, 2024

View reviewed changes

jstourac marked this pull request as draft February 2, 2024 18:37

openshift-ci bot added the do-not-merge/work-in-progress label Feb 2, 2024

jstourac force-pushed the enhanceDockerimageLabels branch from 7d22572 to 1326d63 Compare February 13, 2024 06:03

jstourac marked this pull request as ready for review February 13, 2024 06:03

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 13, 2024

openshift-ci bot requested review from harshad16 and rkpattnaik780 February 13, 2024 06:03

jstourac mentioned this pull request Feb 23, 2024

Add a GitHub action to perform validation of params.env file #429

Merged

3 tasks

openshift-merge-robot added the needs-rebase label Mar 15, 2024

harshad16 marked this pull request as draft May 31, 2024 03:49

openshift-ci bot added the do-not-merge/work-in-progress label May 31, 2024

jstourac closed this Jun 19, 2024

jstourac deleted the enhanceDockerimageLabels branch June 19, 2024 12:51

jstourac restored the enhanceDockerimageLabels branch July 4, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix/enhancement] dockerimage labels check and attempted to deduplicate #309

[Fix/enhancement] dockerimage labels check and attempted to deduplicate #309

jstourac commented Nov 3, 2023 •

edited

Loading

openshift-ci bot commented Nov 3, 2023

atheo89 commented Nov 10, 2023

jstourac commented Dec 7, 2023

jstourac commented Dec 8, 2023

harshad16 left a comment

harshad16 commented Jan 29, 2024

jstourac commented Jan 30, 2024

jstourac Feb 2, 2024 •

edited

Loading

harshad16 Feb 12, 2024

jstourac Feb 13, 2024

harshad16 Feb 13, 2024

jstourac Feb 13, 2024

harshad16 commented Feb 19, 2024

jstourac commented Feb 23, 2024

openshift-merge-robot commented Mar 15, 2024

harshad16 commented May 31, 2024

openshift-ci bot commented May 31, 2024

[Fix/enhancement] dockerimage labels check and attempted to deduplicate #309

[Fix/enhancement] dockerimage labels check and attempted to deduplicate #309

Conversation

jstourac commented Nov 3, 2023 • edited Loading

Description

Notable points for discussion:

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Nov 3, 2023

atheo89 commented Nov 10, 2023

jstourac commented Dec 7, 2023

jstourac commented Dec 8, 2023

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 commented Jan 29, 2024

jstourac commented Jan 30, 2024

jstourac Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

harshad16 Feb 12, 2024

Choose a reason for hiding this comment

jstourac Feb 13, 2024

Choose a reason for hiding this comment

harshad16 Feb 13, 2024

Choose a reason for hiding this comment

jstourac Feb 13, 2024

Choose a reason for hiding this comment

harshad16 commented Feb 19, 2024

jstourac commented Feb 23, 2024

openshift-merge-robot commented Mar 15, 2024

harshad16 commented May 31, 2024

openshift-ci bot commented May 31, 2024

jstourac commented Nov 3, 2023 •

edited

Loading

jstourac Feb 2, 2024 •

edited

Loading