Skip to content

Skip builder image build when it is unmodified #1284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 16, 2023
Merged

Conversation

Molter73
Copy link
Collaborator

@Molter73 Molter73 commented Aug 11, 2023

Description

Skip the builder image build when no changes would make modifications to the actual image (i.e.: no change to third_party libraries and the builder directory itself).

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Testing Performed

  • Run with build-builder-image should force build the builder.
  • Run with a change to either the third_party or builder directories should build the builder. (CI run)
  • Running with no changes to the third_party or builder directories should skip building the builder. (CI run)

@openshift-ci
Copy link

openshift-ci bot commented Aug 11, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions
Copy link

github-actions bot commented Aug 11, 2023

Kernel Method Without Collector Time (secs) With Collector Time (secs) Baseline median (secs) Collector median (secs) PValue
rhel.rhel-8 ebpf 211.374 221.41 204.92 213.7 🟢
ubuntu-os.ubuntu-2004-lts ebpf 240.113 234.556 236.1 234.97 🟢
ubuntu-os.ubuntu-2204-lts ebpf 242.03 244.277 222.56 229.05 🟢

@Molter73 Molter73 force-pushed the mauro/skip-builder branch from 22b1969 to 0213d8b Compare August 14, 2023 13:20
@Molter73 Molter73 marked this pull request as ready for review August 14, 2023 13:53
@Molter73 Molter73 requested a review from a team as a code owner August 14, 2023 13:53
Comment on lines +71 to +77
COLLECTOR_BUILDER_TAG="${DEFAULT_BUILDER_TAG}"
if [[ "${{ github.event_name }}" == 'pull_request' ]]; then
COLLECTOR_BUILDER_TAG="${{ inputs.collector-tag }}"
fi

echo "COLLECTOR_BUILDER_TAG=${COLLECTOR_BUILDER_TAG}" >> "$GITHUB_ENV"
echo "collector-builder-tag=${COLLECTOR_BUILDER_TAG}" >> "$GITHUB_OUTPUT"
Copy link
Contributor

@ovalenti ovalenti Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the builder does not need to be rebuilt, how is the collector-build-tag going to be provided to the collector build ?
Maybe it is empty and then takes the default from Makefile-constants.mk ?
If this is true, maybe it would make sense to not set its value for the "default" case here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll test it, but when skipping I think the output turns into an empty string and our makefiles turn an empty COLLECTOR_BUILDER_TAG env variable to the default (cache):

ifeq ($(COLLECTOR_BUILDER_TAG),)
COLLECTOR_BUILDER_TAG=cache
endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that skipping setting of the value when we are in the default case would be coherent (have the default value's "value" in one place). But, hey, it works !

Copy link
Contributor

@ovalenti ovalenti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to save so much time and resources !

Apart from the nitpick comment for the default value of the builder tag, this look's good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants