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

test: Remove support for Node 18 #30

Merged
merged 1 commit into from
Nov 4, 2024
Merged

test: Remove support for Node 18 #30

Show file tree
Hide file tree
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
1 change: 0 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ jobs:
python-version: ["3.8", "3.12"]
os: [ubuntu-20.04]
toxenv: [django42]
node: [18, 20]
env:
DATA_API_VERSION: "latest"
steps:
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ WORKDIR ${INSIGHTS_CODE_DIR}/
# insights service config commands below
RUN pip install --no-cache-dir -r ${INSIGHTS_CODE_DIR}/requirements/production.txt

RUN nodeenv ${INSIGHTS_NODEENV_DIR} --node=18.20.2 --prebuilt \
&& npm install -g npm@10.5.x
RUN nodeenv ${INSIGHTS_NODEENV_DIR} --node=20.15.1 --prebuilt \
Copy link
Member

@huniafatima-arbi huniafatima-arbi Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As the Dockerfile has been moved to public-dockerfiles repo, node20 change would be needed there. We can remove this change

Copy link
Author

Choose a reason for hiding this comment

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

This Node 20 update is required because the PR drops support for Node 18, and the Dockerfile is still active in this project. There’s a separate PR to remove the Dockerfile, it’s pending due to CI failures, for now Dockerfile is still part of this project and utilized in CI, aligning it with the Node version specified in the PR is necessary to ensure compatibility and prevent CI failures. Once that PR is merged and the Dockerfile is gone this won't matter, until then for as long as Dockerfile continues to exist, this change is relevant and essential.

&& npm install -g npm@10.7.x

# Tried to cache the dependencies by copying related files after the npm install step but npm post install fails in that case.
COPY . ${INSIGHTS_CODE_DIR}/
Expand Down
Loading