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

Dockerfile with nodejs18 for Ubuntu 20.04 #19

Closed
wants to merge 9 commits into from

Conversation

exowanderer
Copy link
Collaborator

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

The Dockerfile originally downloaded the most recent stable LTS version of nodejs.
This caused a yarn and npm errors in the docker build.

The updated Dockerfile downloads specifically version nodejs 18.17.1 via a modified Dockerfile installation process.
Now the yarn install and yarn build commands do not produce errors, and the Vue3 UI displays and behaves as expected.

  • What is the current behavior? (You can also link to an open issue here)
    The Dockerfile originally downloaded the most recent stable LTS version of nodejs.
    This caused a yarn and npm errors in the docker build. See Issue [Bug] Docker build creates yarn error with esbuild #16

  • What is the new behavior (if this is a feature change)?
    It downloads specifically version nodejs 18.17.1 via a modified Dockerfile installation process.
    The yarn install and yarn build commands do not produce errors, and the Vue3 UI displays and behaves as expected.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No. Not breaking change. Only a previous nodejs version.

@exowanderer
Copy link
Collaborator Author

I am not sure if we want to mandate the node 18 for all users, or to detail this in the README as a possible necessary downgrade for Ubuntu 20.04 (Linux Kernel v5) users.

We can bypass this merge if we dont want to downgrade to node18 on all of our systems.

@exowanderer exowanderer mentioned this pull request Jan 31, 2024
4 tasks
@rti
Copy link
Owner

rti commented Feb 1, 2024

I am not sure if we want to mandate the node 18 for all users, or to detail this in the README as a possible necessary downgrade for Ubuntu 20.04 (Linux Kernel v5) users.

We can bypass this merge if we dont want to downgrade to node18 on all of our systems.

I think it is ok to downgrade to 18 for all users now, node 18 is still in maintenance for a while and we do not depend on any features from 20 at the moment. Please checkout #22 for an alternative approach without node version manager.

@andrewtavis
Copy link
Collaborator

Yes I'd agree with keeping it at 18 and not complicating the dependencies. Long term support is good till April 2025 :)

Copy link
Collaborator Author

@exowanderer exowanderer left a comment

Choose a reason for hiding this comment

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

I assume that this PR is outdated because the behavior in the integration branch is cleaner and more functional

@@ -45,20 +46,29 @@ RUN ollama serve & while ! curl http://localhost:11434; do sleep 1; done; ollama
# Setup the custom API and frontend
WORKDIR /workspace
COPY --chmod=644 gswikichat gswikichat
COPY --chmod=755 frontend frontend
COPY --chmod=755 static static
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did we decide to go back to calling the frontend "static", even for Vue3?

Copy link
Owner

Choose a reason for hiding this comment

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

That must have been a merge error. Sorry for that.

RUN curl -sL https://deb.nodesource.com/setup_20.x | bash
RUN apt-get install -y nodejs && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*

RUN npm cache clean -f
RUN npm install -g n
RUN n 18.17.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 assume that this branch PR is no longer relevant because the integration branch is more direct and streamlined than this solution

Copy link
Owner

Choose a reason for hiding this comment

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

Agree.

@rti
Copy link
Owner

rti commented Feb 4, 2024

Covered in #22

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