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

test: Remove support for Node 18 #30

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

BilalQamar95
Copy link

Part of openedx/public-engineering#230.

Description

Completed upgrade to Node 20 by removing the Node 18 CI check and using .nvmrc for version to use.

See openedx/public-engineering#267 for further information.

@BilalQamar95 BilalQamar95 self-assigned this Oct 10, 2024
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@@ -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 [email protected]
RUN nodeenv ${INSIGHTS_NODEENV_DIR} --node=20.15.1 --prebuilt \
Copy link
Member

@huniafatima-arbi huniafatima-arbi Nov 4, 2024

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.

@BilalQamar95 BilalQamar95 merged commit 806013d into master Nov 4, 2024
5 checks passed
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.

2 participants