Skip to content

Add the short SHA to develop containers.#158

Merged
colisee merged 1 commit intoLibreBooking:masterfrom
JohnVillalovos:jlvillal/git_sha
Mar 5, 2026
Merged

Add the short SHA to develop containers.#158
colisee merged 1 commit intoLibreBooking:masterfrom
JohnVillalovos:jlvillal/git_sha

Conversation

@JohnVillalovos
Copy link
Collaborator

@JohnVillalovos JohnVillalovos commented Feb 28, 2026

This is taking advantage of the ability to put a value in config/version-suffix.txt

LibreBooking/librebooking#1093 added that feature

Copilot AI review requested due to automatic review settings February 28, 2026 18:56
@JohnVillalovos JohnVillalovos marked this pull request as draft February 28, 2026 18:56
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 adds the ability to append the LibreBooking short commit SHA to the develop container UI version display, and introduces a GitHub Actions workflow to build the Docker image for pull requests.

Changes:

  • Enhance the Docker build to optionally extract the short SHA from the GitHub tarball directory name and append it to the UI footer (and write build-info.txt).
  • Document the new build arg (APP_GH_ADD_SHA) in BUILD.md and enable it for develop builds.
  • Add a PR CI workflow and plumb a reusable workflow input (appAddSha) through the build/publish pipeline.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Dockerfile Downloads the LibreBooking tarball to derive a short SHA and optionally updates the footer version string.
BUILD.md Documents and wires APP_GH_ADD_SHA for develop vs release builds.
.github/workflows/build_pull_request.yml New workflow to build the image on pull_request.
.github/workflows/build_image_develop.yml Enables SHA appending for scheduled/manual develop image builds via reusable workflow input.
.github/workflows/build_and_publish.yml Adds appAddSha workflow_call input and passes it through as APP_GH_ADD_SHA.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/git_sha branch 2 times, most recently from f4b2542 to 4818f1a Compare February 28, 2026 19:06
@JohnVillalovos JohnVillalovos marked this pull request as ready for review February 28, 2026 19:06
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

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


# Get and customize librebooking
ARG APP_GH_REF
ARG APP_GH_ADD_SHA=false
Copy link
Collaborator

@colisee colisee Mar 1, 2026

Choose a reason for hiding this comment

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

Argument APP_GH_ADD_SHA is not really needed, for its value depends on the value of argument APP_GH_REF:

APP_GH_REF APP_GH_ADD_SHA
refs/tags/* false
refs/heads/develop true

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 did that at first but I have plans at some point to move development to the main branch. So I didn't want to embed the logic of deciding if something is a development versus release based on the name.

I thought it would be better to be explicit.

It seems like APP_GH_REF passes values like v4.1.0 and develop. Does it use values like refs/heads/develop?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Argument APP_GH_REF always contains the refs/* path.
Henceforth argument APP_GH_ADD_SHA can be derived as follows:

  • APP_GH_REF starts with refs/tags -> this is a release and APP_GH_ADD_SHA = false
  • APP_GH_REF starts with refs/heads -> this is the "development" version and APP_GH_ADD_SHA = true

If you are OK with the above, then you can drop the APP_GH_ADD_SHA argument

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argument APP_GH_REF always contains the refs/* path.

I don’t think this is accurate in the current setup. In our GitHub workflows, APP_GH_REF is passed from appGitRefs, and callers currently provide values like develop and v4.2.0 (not refs/*).

Even if it did contain the refs/*, I still personally would prefer to have the APP_GH_ADD_SHA value as it allows it to be a setting instead of trying to programmatically determine what is the intention.

uses: ./.github/workflows/build_and_publish.yml
with:
appGitRefs: develop
appAddSha: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

The value of appAddSha can be derived from appGitRefs:

appGitrefs appAddSha
develop true
v* false

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 did that at first but I have plans at some point to move development to the main branch. So I didn't want to embed the logic of deciding if something is a development versus release based on the name.

I thought it would be better to be explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adapt, based on your final decision regarding Dockerfile after my last comment

Copy link
Collaborator Author

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback. I will work on incorporating that into the next version

uses: ./.github/workflows/build_and_publish.yml
with:
appGitRefs: develop
appAddSha: true
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 did that at first but I have plans at some point to move development to the main branch. So I didn't want to embed the logic of deciding if something is a development versus release based on the name.

I thought it would be better to be explicit.


# Get and customize librebooking
ARG APP_GH_REF
ARG APP_GH_ADD_SHA=false
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 did that at first but I have plans at some point to move development to the main branch. So I didn't want to embed the logic of deciding if something is a development versus release based on the name.

I thought it would be better to be explicit.

It seems like APP_GH_REF passes values like v4.1.0 and develop. Does it use values like refs/heads/develop?

@JohnVillalovos JohnVillalovos changed the title Add the short SHA to develop containers. Also setup a CI to run for Pull Requests. Add the short SHA to develop containers. Mar 1, 2026
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/git_sha branch 2 times, most recently from 79a5d67 to f1d6a46 Compare March 1, 2026 20:13
@ikke-t
Copy link
Collaborator

ikke-t commented Mar 1, 2026

As said, I don't know github actions, but I looked at the shell script parts. Looks good to me.

Dockerfile Outdated
| sed -nE 's/.*filename="?([^";]+)"?.*/\1/p' \
| tail -n1); \
echo TARBALL_FILENAME ${TARBALL_FILENAME}; \
LB_SHORT_SHA=$(echo $TARBALL_FILENAME | sed -E 's/.*-g([0-9a-f]+)\.tar\.gz/\1/'); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add the | sed -E 's/.*-g([0-9a-f]+)\.tar\.gz/\1/' to the previous long pipe statement, so that the hash is done in one shot

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 kind of like it this way as it makes it easier to understand for people, IMHO.

Dockerfile Outdated
LB_SHORT_SHA=$(echo $TARBALL_FILENAME | sed -E 's/.*-g([0-9a-f]+)\.tar\.gz/\1/'); \
if [ -n "${LB_SHORT_SHA}" ]; then \
printf '%s\n' "${LB_SHORT_SHA}" > /var/www/html/config/version-suffix.txt; \
else \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would drop the else part of the if block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Showing that there was an error could be useful in the future if something stops working.

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/git_sha branch 2 times, most recently from 2747f3d to 046a67a Compare March 5, 2026 00:01
Copy link
Collaborator Author

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

@colisee

I rebased the PR with the latest changes and also replied to your feedback. Please let me know if you disagree with my opinions and I can change it.


# Get and customize librebooking
ARG APP_GH_REF
ARG APP_GH_ADD_SHA=false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argument APP_GH_REF always contains the refs/* path.

I don’t think this is accurate in the current setup. In our GitHub workflows, APP_GH_REF is passed from appGitRefs, and callers currently provide values like develop and v4.2.0 (not refs/*).

Even if it did contain the refs/*, I still personally would prefer to have the APP_GH_ADD_SHA value as it allows it to be a setting instead of trying to programmatically determine what is the intention.

Dockerfile Outdated
| sed -nE 's/.*filename="?([^";]+)"?.*/\1/p' \
| tail -n1); \
echo TARBALL_FILENAME ${TARBALL_FILENAME}; \
LB_SHORT_SHA=$(echo $TARBALL_FILENAME | sed -E 's/.*-g([0-9a-f]+)\.tar\.gz/\1/'); \
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 kind of like it this way as it makes it easier to understand for people, IMHO.

Dockerfile Outdated
LB_SHORT_SHA=$(echo $TARBALL_FILENAME | sed -E 's/.*-g([0-9a-f]+)\.tar\.gz/\1/'); \
if [ -n "${LB_SHORT_SHA}" ]; then \
printf '%s\n' "${LB_SHORT_SHA}" > /var/www/html/config/version-suffix.txt; \
else \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Showing that there was an error could be useful in the future if something stops working.

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +84 to +97
if [ "${APP_GH_ADD_SHA}" = "true" ]; then
LB_SHORT_SHA=""
# TARBALL_FILENAME will be like the result of a `git describe` For
# example: 'LibreBooking-librebooking-v4.1.0-126-g6cc8a4c.tar.gz' where
# 'g6cc8a4c' is the short SHA prefixed with 'g'. So the short SHA is
# '6cc8a4c'
TARBALL_FILENAME=$(\
curl \
--head \
--fail \
--silent \
--show-error \
--location ${LB_TARBALL_URL} \
| sed -nE 's/.*filename="?([^";]+)"?.*/\1/p')
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

There is a potential race condition: the tarball content is downloaded on lines 79-83, and then a separate HEAD request is made on lines 90-97 to determine the SHA. If a push to the branch occurs between these two requests, the HEAD response could report a different commit SHA than what was actually extracted. This would result in the version suffix not matching the actual code in the container.

Consider extracting the SHA from the first curl response headers instead (e.g., using curl -D to dump headers while simultaneously piping the body to tar), or alternatively, extracting the top-level directory name from the tarball itself (which contains the SHA) before extraction.

Copilot uses AI. Check for mistakes.
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 feel like this is a low chance of happening and also a low impact if it did happen

@colisee colisee added the enhancement New feature or request label Mar 5, 2026
@colisee colisee merged commit 26a5247 into LibreBooking:master Mar 5, 2026
1 check passed
@JohnVillalovos JohnVillalovos deleted the jlvillal/git_sha branch March 5, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants