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

Address warning message in the CI pipelines #942

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

atrendafilov
Copy link
Contributor

@atrendafilov atrendafilov commented Nov 8, 2023

Issue

Github CI pipelines action is completed successfully, but with warning about outdated Node.js version

Solution

Upgrade default Node.js version in the Ubuntu image from 12 to 16

Testing done

Successfully tested

Fix #939

Upgrade default Node.js version to 16

Resolves a warning about Node.js 12 in the build action by upgrading the default Node.js version to 16 in the Ubuntu image.

Changes to be committed:
	modified:   .github/workflows/pull-request.yaml
@atrendafilov atrendafilov self-assigned this Nov 8, 2023
@recena recena changed the title [Issue-939] Fixes warning message in the CI pipelines Fixes warning message in the CI pipelines Nov 8, 2023
@recena recena changed the title Fixes warning message in the CI pipelines Address warning message in the CI pipelines Nov 8, 2023
@recena
Copy link
Collaborator

recena commented Nov 8, 2023

@atrendafilov In this PR you are updating one pipeline but there are three pipelines. Is there any reason not to update the other?

@@ -12,6 +12,9 @@ jobs:
CI_TOOLS_URL: https://github.com/samuelattwood/partner-charts-ci/releases/latest/download/partner-charts-ci-linux-amd64

steps:
- uses: actions/setup-node@v2
with:
node-version: '16.x'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not node-version: 18?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

version 16 is sufficient enough and I didn't want to make a huge leap in versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -12,6 +12,9 @@ jobs:
CI_TOOLS_URL: https://github.com/samuelattwood/partner-charts-ci/releases/latest/download/partner-charts-ci-linux-amd64

steps:
- uses: actions/setup-node@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not the latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as the above comment about the versions

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the value of not using the latest version of the official Github Actions.

@@ -12,6 +12,9 @@ jobs:
CI_TOOLS_URL: https://github.com/samuelattwood/partner-charts-ci/releases/latest/download/partner-charts-ci-linux-amd64

steps:
- uses: actions/setup-node@v2
with:
node-version: '16.x'
- uses: actions/checkout@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the fix is to update this action to the latest version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm almost sure this fix the warning message. More references actions/checkout#1047 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

actions/checkout@v2 uses Node12.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be a possible fix, but I'm not familiar with the reason behind the use of actions/checkout@v2 in this pipeline, so just to be on the save side I have opted out for this solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@atrendafilov This is the fix according to the documentation and references. In fact, we don't need to add a new step.

@atrendafilov
Copy link
Contributor Author

@atrendafilov In this PR you are updating one pipeline but there are three pipelines. Is there any reason not to update the other?

Because in the other pipelines we use actions/checkout@v3 which mitigates this issue.

Copy link
Collaborator

@recena recena left a comment

Choose a reason for hiding this comment

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

I would go through updating actions/checkout@v2

@recena
Copy link
Collaborator

recena commented Nov 10, 2023

Merging to avoid conflicts.

@recena recena merged commit 87f7d46 into main-source Nov 10, 2023
1 check 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.

Address warning message in the CI pipelines
2 participants