Skip to content

Conversation

@aorcholski
Copy link
Contributor

@aorcholski aorcholski commented Dec 2, 2025

DAQ-12536

Description

This PR removes unused functions from the prepare-build-variables.sh script. The script is also removed from the CI workflow.

I added metadata action to keep the tags logic in one place. It returns both values provided by docker/metada-action:

tags 	    String 	Docker tags
tag-names 	String 	Docker tag names without image base name

Sanitized names are used in the tags logic because CVS parser called by docker/metada-action skips all characters after # (it is a comment).

How can this be tested?

I used forked repo. I created new workflow based on the CI one that pushes images to a different registry. I was able to compare images.

@aorcholski aorcholski force-pushed the feature/prep-build-var branch 4 times, most recently from 5541d7e to e65907d Compare December 4, 2025 20:10
@aorcholski aorcholski force-pushed the feature/prep-build-var branch 2 times, most recently from 60c70bb to fa7d58e Compare December 10, 2025 12:08
@aorcholski aorcholski changed the title WIP Improve prepare-build-variables script Remove unused functions in prepare-build-variables.sh script Dec 10, 2025
@aorcholski aorcholski marked this pull request as ready for review December 10, 2025 12:39
@aorcholski aorcholski requested a review from a team as a code owner December 10, 2025 12:39
shell: bash
run: |
# Sanitize names
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could you please remove this newline?

Suggested change

id: set-build-date
run: |
# Set build date
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@Incredible-O Incredible-O requested a review from Copilot December 11, 2025 09:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes unused functions and workflow steps from the CI build process by consolidating Docker image metadata generation into a reusable action. The changes eliminate the deprecated prepare-build-variables.sh script and its associated workflow job, replacing them with a new metadata action that centralizes tag and label generation logic.

Key changes:

  • Removed unused create_docker_image_labels() function and related output variables from prepare-build-variables.sh
  • Eliminated the prepare job from the CI workflow
  • Created a new reusable metadata action that generates Docker image metadata including tags, labels, and annotations

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
hack/build/ci/prepare-build-variables.sh Removed unused create_docker_image_labels() function and related variable assignments
.github/workflows/ci.yaml Removed the prepare job and updated preflight-main to use new action inputs
.github/actions/preflight/action.yaml Refactored to use the new metadata action instead of separate version/registry/repository inputs
.github/actions/metadata/action.yaml New reusable action that consolidates Docker metadata generation with sanitized branch names
.github/actions/build-push-image/action.yaml Integrated the new metadata action and simplified the build process

type=raw,value=nightly,enable=${{ github.event_name == 'schedule' }}
# tags
type=raw,value=${{ steps.sanitize.outputs.ref_name_without_prefix }},enable=${{ github.ref_type == 'tag' }}
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

For tags, the original logic in create_docker_image_tag() returns github.ref_name directly without stripping the 'v' prefix. This new implementation strips the prefix unconditionally, which changes the tag format for version tags and may break existing references or tooling that expects tags to include the 'v' prefix.

Suggested change
type=raw,value=${{ steps.sanitize.outputs.ref_name_without_prefix }},enable=${{ github.ref_type == 'tag' }}
type=raw,value=${{ steps.sanitize.outputs.ref_name }},enable=${{ github.ref_type == 'tag' }}

Copilot uses AI. Check for mistakes.
annotations: |
${{ inputs.annotation }}
version=${{ github.event_name == 'pull_request' && github.head_ref || github.ref_name }}
go_linker_args=$(hack/build/create_go_linker_args.sh "${{ steps.meta.outputs.tag-names }}" "${{ github.sha }}")
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The create_go_linker_args.sh script is being passed tag-names (which contains multiple comma-separated tags without the image base name) instead of a single tag value. The original code passed docker_image_tag from create_docker_image_tag(), which was a single tag string. This may cause the linker args script to fail or produce incorrect version information.

Suggested change
go_linker_args=$(hack/build/create_go_linker_args.sh "${{ steps.meta.outputs.tag-names }}" "${{ github.sha }}")
go_linker_args=$(hack/build/create_go_linker_args.sh "${{ steps.meta.outputs.tag }}" "${{ github.sha }}")

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

create_go_linker_args.sh script needs tag without the image base name so it should be value of tag-names .

@aorcholski aorcholski force-pushed the feature/prep-build-var branch from 5774a0b to 761667f Compare December 12, 2025 14:07
@aorcholski aorcholski force-pushed the feature/prep-build-var branch from 761667f to 7002e17 Compare December 12, 2025 14:09
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.

3 participants