opendatahub-io / notebooks

Notebook images for ODH
Apache License 2.0
15 stars 51 forks source link

Limit PR checks to build only the modified images #558

Closed jiridanek closed 1 month ago

jiridanek commented 1 month ago

Description

Followup to

that filters out the list of images to build during a PR check only to those that have some source files changed.

This will speed up the PR check build.

How Has This Been Tested?

Created a draft PR that changes one file in a single docker image

Checked that it correctly builds all images that depend on this one.

Merge criteria:

jiridanek commented 1 month ago

Review comments history


I see on this draft that you affect only the DS notebook, but i see that GH runs the trusty and some others, do i miss something? https://github.com/opendatahub-io/notebooks/pull/555 stiill checking


actually, no, it is not nonsense, it truly is all images that 1) are at the end of the FROM: dependency chain and 2) they or one of the images in their FROM: chain is built in jupyter/datascience/ubi9-python-3.9 directory

for example, here's why it builds cuda; that cuda image builds in the jupyter/datascience/ubi9-python-3.9 directory

# Build and push cuda-jupyter-datascience-ubi9-python-3.9 image to the registry
.PHONY: cuda-jupyter-datascience-ubi9-python-3.9
cuda-jupyter-datascience-ubi9-python-3.9: cuda-jupyter-minimal-ubi9-python-3.9
    $(call image,$@,jupyter/datascience/ubi9-python-3.9,$<)

and here's why it builds trusty; trusty image depends on jupyter-datascience-ubi9-python-3.9

# Build and push jupyter-trustyai-ubi9-python-3.9 image to the registry
.PHONY: jupyter-trustyai-ubi9-python-3.9
jupyter-trustyai-ubi9-python-3.9: jupyter-datascience-ubi9-python-3.9
    $(call image,$@,jupyter/trustyai/ubi9-python-3.9,$<)

Since you affect the DS, I would expect to build the base, minimal and DS (base & minimal are prior DS on the build chain)


when it builds each of the three images, it does the full chain, so it starts from the base, https://github.com/opendatahub-io/notebooks/actions/runs/9498328161/job/26176822339?pr=555

It has to do the full chain because I don't have image registry write access available when building PRs on github actions

jiridanek commented 1 month ago

@atheo89 @harshad16 can you create tide/merge-method-squash label in this repo? It would be useful here. I want all commits squashed, but I don't want to cause any more notification spam by force-pushing.

If I could apply the label, tide would squash everything when it does its own thing.

Tide docs, https://www.kubernetes.dev/docs/guide/pull-requests/#squashing, and here's it in action https://github.com/kubernetes-sigs/prow/pulls?q=is%3Apr+is%3Aopen+label%3Atide%2Fmerge-method-squash

openshift-ci[bot] commented 1 month ago

New changes are detected. LGTM label has been removed.

atheo89 commented 1 month ago

Can we implement filtering to avoid building images for certain file types, such as .md or .yaml? Probably as future enhancement :thinking: This idea came to me when I opened this PR:: https://github.com/opendatahub-io/notebooks/pull/573

openshift-ci[bot] commented 1 month ago

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

Test name Commit Details Required Rerun command
ci/prow/notebooks-e2e-tests 60587154f2dd9334f2e7eeee877379bd28ef7b5b link true /test notebooks-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
jiridanek commented 1 month ago

@atheo89 future enhancement

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: jstourac

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

The pull request process is described here

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/opendatahub-io/notebooks/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
atheo89 commented 1 month ago

/override ci/prow/runtimes-ubi9-e2e-tests /override ci/prow/runtimes-ubi8-e2e-tests /override ci/prow/anaconda-ubi8-e2e-tests /override ci/prow/codeserver-notebook-e2e-tests /override ci/prow/intel-notebooks-e2e-tests /override ci/prow/notebooks-ubi8-e2e-tests /override ci/prow/notebooks-ubi9-e2e-tests /override ci/prow/rstudio-notebook-e2e-tests

openshift-ci[bot] commented 1 month ago

@atheo89: Overrode contexts on behalf of atheo89: ci/prow/anaconda-ubi8-e2e-tests, ci/prow/codeserver-notebook-e2e-tests, ci/prow/intel-notebooks-e2e-tests, ci/prow/notebooks-ubi8-e2e-tests, ci/prow/notebooks-ubi9-e2e-tests, ci/prow/rstudio-notebook-e2e-tests, ci/prow/runtimes-ubi8-e2e-tests, ci/prow/runtimes-ubi9-e2e-tests

In response to [this](https://github.com/opendatahub-io/notebooks/pull/558#issuecomment-2191265384): >/override ci/prow/runtimes-ubi9-e2e-tests >/override ci/prow/runtimes-ubi8-e2e-tests >/override ci/prow/anaconda-ubi8-e2e-tests >/override ci/prow/codeserver-notebook-e2e-tests >/override ci/prow/intel-notebooks-e2e-tests >/override ci/prow/notebooks-ubi8-e2e-tests >/override ci/prow/notebooks-ubi9-e2e-tests >/override ci/prow/rstudio-notebook-e2e-tests > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
openshift-ci[bot] commented 1 month ago

New changes are detected. LGTM label has been removed.