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.

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

fix(server): Add lerc external into server package. #3348

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Wentao-Kuang
Copy link
Contributor

@Wentao-Kuang Wentao-Kuang commented Sep 26, 2024

Motivation

It looks like we are missing lerc in the server cli.
https://github.com/linz/basemaps-config/actions/runs/11061552896/job/30734619452?pr=970
image

Modifications

Add the lerc into server and cli package

Verification

Local build server container with lerc uninstall vs installed.

UNINSTALL

image

INSTALLED

image

@@ -12,6 +12,8 @@ WORKDIR /app/basemaps
COPY ./basemaps-landing*.tgz /app/basemaps/
COPY ./basemaps-server*.tgz /app/basemaps/

RUN npm install lerc@^4.0.4
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed here? if we were missing binary deps then sharp should be missing too?

Copy link
Contributor Author

@Wentao-Kuang Wentao-Kuang Sep 27, 2024

Choose a reason for hiding this comment

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

I didn't see an error related sharp when I test the server container with screenshots. Might be the deps for cogify in the cli container? But we definitely need to install lerc in here to make it work.

@@ -15,7 +15,7 @@ RUN apt-get update
RUN apt-get install -y nodejs

# Install sharp TODO update this when we change sharp versions
RUN npm install sharp@0.33.0
RUN npm install sharp@0.33.0 lerc@^4.0.4
Copy link
Member

Choose a reason for hiding this comment

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

where in the CLI is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this cli container for the server in basemaps-config. So we need to install lerc this into cli container.

Copy link
Member

Choose a reason for hiding this comment

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

if the server is being installed it should come with lerc so I think the server is just missing the lerc dep? how is sharp defined in basemaps/server ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sharp is installed as dependency already in the basemaps/server

But we are not installing sharp in the basemaps/cli, so we installed into container directly.

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 don't think server package is installed in the container, it is just a function call to the package/server, so we still need to install lerc in the cli container.

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

Successfully merging this pull request may close these issues.

2 participants