Skip to content
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.

Sign up for GitHub

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

use invoiceninja.tar.gz #717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −4
Open

use invoiceninja.tar.gz #717

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions alpine/5/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@ FROM --platform=$BUILDPLATFORM node:lts-alpine as nodebuild
# Download Invoice Ninja
ARG INVOICENINJA_VERSION
ARG REPOSITORY=invoiceninja/invoiceninja
ARG FILENAME=invoiceninja.tar
ARG FILENAME=invoiceninja.tar.gz

RUN set -eux; apk add curl unzip grep

RUN DOWNLOAD_URL=$(curl -s "https://api.github.com/repos/invoiceninja/invoiceninja/releases/latest" | grep -o '"browser_download_url": "[^"]*invoiceninja.tar"' | cut -d '"' -f 4) && \
RUN DOWNLOAD_URL=$(curl -s "https://api.github.com/repos/invoiceninja/invoiceninja/releases/latest" | grep -o '"browser_download_url": "[^"]*invoiceninja.tar.gz"' | cut -d '"' -f 4) && \
curl -LJO "$DOWNLOAD_URL" && \
mv invoiceninja.tar /tmp/ninja.tar
mv invoiceninja.tar.gz /tmp/ninja.tar.gz

# Extract Invoice Ninja
RUN mkdir -p /var/www/app \
&& tar -xvf /tmp/ninja.tar -C /var/www/app/ \
&& tar -xvf /tmp/ninja.tar.gz --strip-components=1 -C /var/www/app/ \
Copy link
Member

Choose a reason for hiding this comment

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

why are we introducing the strip-components=1 here? this will change the directory levels?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • ninja.tar directly contains the gzipped files.
  • ninja.tar.gz contains an additional folder with the gzipped files. This is aligned with standard conventions. So if someone extracts it careless ly, it will not by default extract into the current directory, which may contain already files, but in a folder in the current directory. So tar.gz with strip-components is identical to tar without strip-components

Copy link
Member

Choose a reason for hiding this comment

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

This impacts the selfupdater which is expecting there to be no nested directory. PharData ->extractTo() cannot handle this particular scenario and would require extracting to a temp dir and then copying file back.

Also, changing the directory structure will impact other third parties that use our release files for their own builds.

Copy link
Collaborator Author

@benbrummer benbrummer Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The release artifact invoiceninja.tar is at the moment still available and the selfupdater is not used in docker.

Proposal:
We should get rid of invoiceninja.tar:

  • Unexpected/non-standard behaviour of extracting by default into the current directory
  • wrong extension, as it is a gzip archive

So removing https://github.com/invoiceninja/invoiceninja/blob/99d2652f89b64c49be3aefe420500e652e50bbaf/.github/workflows/react_release.yml#L66 needs than be postponed, until consumers of invoiceninja.tar had some time switch to the new artifact.

But if it is not possible to handle this for the selfupdater, it makes no sense.

Copy link
Member

Choose a reason for hiding this comment

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

@benbrummer there are two issues here:

  1. Dealing with the additional folder in selfupdate controller - there is no graceful way to handle this.
  2. Third parties that have build assuming no additional directory is present

My preference here is to maintain the current directory structure so that it is not a breaking change for many users.

&& mkdir -p /var/www/app/public/logo /var/www/app/storage

WORKDIR /var/www/app
Expand Down
Loading