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

Gslux 635 use v4 release bundle #3180

Open
wants to merge 4 commits into
base: integration-vue
Choose a base branch
from
Open

Gslux 635 use v4 release bundle #3180

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29fd8d1
reference released bundle in v4 repo
mki-c2c Jul 30, 2024
21135b0
install cleanly via npm
mki-c2c Jul 30, 2024
fbc3f6c
update release reference to the bundle
mki-c2c Jul 30, 2024
283e3dc
use release url in package.json
mki-c2c Aug 1, 2024
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
10 changes: 8 additions & 2 deletions geoportal/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ COPY geoportailv3_geoportal/static-ngeo /app/geoportailv3_geoportal/static-ngeo
RUN rm -rf /usr/lib/node_modules/ngeo
RUN mv /app/geoportailv3_geoportal/static-ngeo/ngeo /usr/lib/node_modules/ngeo

COPY . /app
RUN mkdir -p /app
COPY ./package.json /app

# jsapi generation
ADD ./jsapi /etc/apiv4/
Comment on lines 38 to 43
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 we can get rid of the diff of the Dockerfile completely, as the npm install in between the two changes only concerns the JS API, if I see correctly. The package.json for the app is actually already copied in line 28.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main point of this diff is removing the COPY . /app and putting it after the jsapi build
but yes, we can remove

RUN mkdir -p /app
COPY ./package.json /app

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I don't get the advantage of moving COPY . /app, neither as the WORKDIR changes, but I might be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By moving the cp . as late as possible we avoid all unnecessary docker build steps (npm install for jsapi and build jsapi) if the source code changes
This is just a build optimization which may reduce the image build time if something changes in app

We can leave the Dockerfile as is if that is more comfortable for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I'm fine with moving the COPY . /app

Expand All @@ -45,17 +46,22 @@ RUN node --version
RUN npm install --no-optional && npm cache clear --force
RUN /etc/apiv4/rebuild_api.sh

COPY . /app
WORKDIR /app
# sad fix, to allow webpack's file-loader to find files with query string & hash added
RUN ln -s /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-regular-400.eot /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-regular-400.eot?#iefix && \
ln -s /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-regular-400.svg /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-regular-400.svg#fontawesome && \
ln -s /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-solid-900.eot /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-solid-900.eot?#iefix && \
ln -s /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-solid-900.svg /usr/lib/node_modules/@fortawesome/fontawesome-free/webfonts/fa-solid-900.svg#fontawesome


# RUN make checks
RUN make build

RUN curl -L https://github.com/Geoportail-Luxembourg/luxembourg-geoportail/releases/download/$(cat v4_release.txt)/bundle.tgz > /tmp/bundle.tgz && \
tar -xzvf /tmp/bundle.tgz -C /tmp && \
mkdir -p /usr/lib/node_modules/luxembourg-geoportail && \
mv /tmp/bundle/ /usr/lib/node_modules/luxembourg-geoportail
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of manipulating the npm dependencies "manually" here, as the package.json is already there to manage such things. Maybe we could find a way to continue making the bundle available that is directly accessible for the package.json (in a dedicated branch or via npm for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.
There are three arguments in favor of this approach, though:

  • it's "less bad" than before with the bundle inside the repo and the whole source files installed and unused
  • it's only for the time of the backport while v3 and v4 coexist. And it's by far not the only hack for this time
  • in case of a V4 change, the docker image only has to be rebuilt for the last stages, the (long) installation of basic V3 npm dependencies can be skipped

so in the end I do not feel too bad with this "hack", but I'll check if we can build a "fake" npm package to install via the git hash...

Copy link

Choose a reason for hiding this comment

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

Agree with @tkohr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks for your great idea, so we can use npm to install the bundle, that gives more transparency


RUN make apps
RUN mv webpack.apps.js webpack.apps.js.tmpl
# copy web component styles and translations from bundle to static-ngeo
Expand Down
1 change: 1 addition & 0 deletions geoportal/v4_release.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
GSLUX-635_5278bb4fbd7496cbb092087899d106256e3154a0