Limit PR checks to build only the modified images #558

jiridanek · 2024-06-13T08:22:18Z

Description

Followup to

RHOAIENG-7525: Build opendatahub-io/notebooks in GitHub Action with caching #543

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

Draft PR to check that #558 changes work correctly in a PR that actually modifies some images #555

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

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

atheo89

Added a comment

ci/cached-builds/gha_pr_changed_files.py

caponetto

Hey @jiridanek, very nice improvement to save resources!
I've left some comments, please check if they make sense.

caponetto · 2024-06-13T11:44:18Z

.github/workflows/build-notebooks-pr.yaml

 "on":
  "pull_request":

+permissions:
+  contents: read
+  packages: read


I see we don't interact with GitHub Packages in this repository, so maybe the packages permission is not necessary here. I know it was there before, but just pointing it out.

packages: read is necessary to use ghcr.io repository as a cache; it is running podman with

--cache-from ghcr.io/opendatahub-io/notebooks/workbench-images/build-cache

looking at the trial PR, it does not seem to be pulling from cache, not even the base images which should be completely unchanged! https://github.com/opendatahub-io/notebooks/actions/runs/9498328161/job/26176821937?pr=555

In the recent PR from Diamond it is using the cache just fine; not sure what busted the cache here https://github.com/opendatahub-io/notebooks/actions/runs/9490922279/job/26155372847?pr=557

it's not pulling from cache because the FROM registry.access.redhat.com/ubi9/python-39:latest image has been updated in the meantime! that explains things, mystery solved, never mind me

Nice! Thanks for the info :)

.github/workflows/build-notebooks-pr.yaml

caponetto · 2024-06-13T11:47:25Z

ci/cached-builds/gha_pr_changed_files.py

+    while CURSOR is not None:
+        request = compose_gh_api_request(pull_number=pr_number, owner=owner, repo=repo, per_page=per_page, cursor=CURSOR)
+        response = urllib.request.urlopen(request)
+        data = json.loads(response.read().decode("utf-8"))
+        response.close()
+        edges = data["data"]["repository"]["pullRequest"]["files"]["edges"]
+
+        CURSOR = None
+        for edge in edges:
+            files.append(edge["node"]["path"])
+            CURSOR = edge["cursor"]


I think it would be more efficient to get the changed files using git diff.

maybe? golangci action queries github api for it, see their docs for only-new-issues, https://github.com/golangci/golangci-lint-action?tab=readme-ov-file#only-new-issues

I stole this from them 🙃

the gh api query means I can fetch only the latest revision when pulling from git; and I don't have to worry about figuring out what's the PR base commit (I'd need to ask GH for that anyways, although that's available in the github context right away https://docs.github.com/en/actions/learn-github-actions/contexts#github-context)

jiridanek · 2024-06-13T11:53:26Z

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? #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 · 2024-06-13T13:37:25Z

@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

jstourac

One nitpick, otherwise LGTM.

ci/cached-builds/gha_pr_changed_files.py

openshift-ci · 2024-06-15T15:04:27Z

New changes are detected. LGTM label has been removed.

atheo89 · 2024-06-18T10:35:24Z

Can we implement filtering to avoid building images for certain file types, such as *.md or *.yaml? Probably as future enhancement
🤔 This idea came to me when I opened this PR:: #573

openshift-ci · 2024-06-21T15:04:51Z

@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	`6058715`	link	true	`/test notebooks-e2e-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.

jiridanek · 2024-06-21T16:27:47Z

@atheo89 future enhancement

Can we implement filtering to avoid building images for certain file types, such as *.md or *.yaml? Probably as future enhancement #581

openshift-ci · 2024-06-26T09:41:02Z

[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

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

atheo89 · 2024-06-26T09:43:09Z

/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 · 2024-06-26T09:43:27Z

@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:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2024-06-26T09:44:07Z

New changes are detected. LGTM label has been removed.

(cherry picked from commit 7bfbf32)

openshift-ci bot requested review from dibryant and harshad16 June 13, 2024 08:22

jiridanek mentioned this pull request Jun 13, 2024

Draft PR to check that #558 changes work correctly in a PR that actually modifies some images #555

Closed

3 tasks

jiridanek requested review from atheo89 and jstourac June 13, 2024 08:31

atheo89 reviewed Jun 13, 2024

View reviewed changes

ci/cached-builds/gha_pr_changed_files.py Outdated Show resolved Hide resolved

caponetto reviewed Jun 13, 2024

View reviewed changes

jiridanek force-pushed the jd_try_dep_inside_nodes branch from bafcf10 to 0ac42b4 Compare June 13, 2024 13:01

jiridanek mentioned this pull request Jun 14, 2024

Maybe get a list of changed files using git diff and pass them to the script? #560

Closed

jstourac approved these changes Jun 15, 2024

View reviewed changes

ci/cached-builds/gha_pr_changed_files.py Outdated Show resolved Hide resolved

openshift-ci bot assigned jstourac Jun 15, 2024

openshift-ci bot added the lgtm label Jun 15, 2024

jiridanek force-pushed the jd_try_dep_inside_nodes branch from 0ac42b4 to 6058715 Compare June 15, 2024 15:04

openshift-ci bot removed the lgtm label Jun 15, 2024

jiridanek added the lgtm label Jun 15, 2024

jstourac approved these changes Jun 15, 2024

View reviewed changes

jiridanek added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 21, 2024

jiridanek mentioned this pull request Jun 21, 2024

Can we implement filtering to avoid building images for certain file types, such as *.md or *.yaml? Probably as future enhancement #581

Closed

atheo89 added the approved label Jun 26, 2024

Limit PR checks to build only the modified images

6cf2725

jiridanek force-pushed the jd_try_dep_inside_nodes branch from 6058715 to 6cf2725 Compare June 26, 2024 09:44

openshift-ci bot removed the lgtm label Jun 26, 2024

atheo89 added the lgtm label Jun 26, 2024

openshift-merge-bot bot merged commit 7bfbf32 into opendatahub-io:main Jun 26, 2024
6 checks passed

jiridanek mentioned this pull request Jun 30, 2024

ci: filter GitHub Actions PR build by changed files and .dockerignores #604

Closed

3 tasks

jstourac pushed a commit to jstourac/notebooks that referenced this pull request Jul 19, 2024

Limit PR checks to build only the modified images (opendatahub-io#558)

3f059f0

(cherry picked from commit 7bfbf32)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit PR checks to build only the modified images #558

Limit PR checks to build only the modified images #558

jiridanek commented Jun 13, 2024 •

edited

Loading

atheo89 left a comment

caponetto left a comment

caponetto Jun 13, 2024

jiridanek Jun 13, 2024

jiridanek Jun 13, 2024

jiridanek Jun 13, 2024

caponetto Jun 13, 2024

caponetto Jun 13, 2024

jiridanek Jun 13, 2024 •

edited

Loading

jiridanek commented Jun 13, 2024 •

edited

Loading

jiridanek commented Jun 13, 2024 •

edited

Loading

jstourac left a comment

openshift-ci bot commented Jun 15, 2024

atheo89 commented Jun 18, 2024 •

edited

Loading

openshift-ci bot commented Jun 21, 2024 •

edited

Loading

jiridanek commented Jun 21, 2024

openshift-ci bot commented Jun 26, 2024

atheo89 commented Jun 26, 2024

openshift-ci bot commented Jun 26, 2024

openshift-ci bot commented Jun 26, 2024

Limit PR checks to build only the modified images #558

Limit PR checks to build only the modified images #558

Conversation

jiridanek commented Jun 13, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

atheo89 left a comment

Choose a reason for hiding this comment

caponetto left a comment

Choose a reason for hiding this comment

caponetto Jun 13, 2024

Choose a reason for hiding this comment

jiridanek Jun 13, 2024

Choose a reason for hiding this comment

jiridanek Jun 13, 2024

Choose a reason for hiding this comment

jiridanek Jun 13, 2024

Choose a reason for hiding this comment

caponetto Jun 13, 2024

Choose a reason for hiding this comment

caponetto Jun 13, 2024

Choose a reason for hiding this comment

jiridanek Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

jiridanek commented Jun 13, 2024 • edited Loading

jiridanek commented Jun 13, 2024 • edited Loading

jstourac left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 15, 2024

atheo89 commented Jun 18, 2024 • edited Loading

openshift-ci bot commented Jun 21, 2024 • edited Loading

jiridanek commented Jun 21, 2024

openshift-ci bot commented Jun 26, 2024

atheo89 commented Jun 26, 2024

openshift-ci bot commented Jun 26, 2024

openshift-ci bot commented Jun 26, 2024

jiridanek commented Jun 13, 2024 •

edited

Loading

jiridanek Jun 13, 2024 •

edited

Loading

jiridanek commented Jun 13, 2024 •

edited

Loading

jiridanek commented Jun 13, 2024 •

edited

Loading

atheo89 commented Jun 18, 2024 •

edited

Loading

openshift-ci bot commented Jun 21, 2024 •

edited

Loading