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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃Ч Switch to Node 20 LTS #2557

Merged
merged 1 commit into from Mar 13, 2024

Conversation

cardil
Copy link
Member

@cardil cardil commented Mar 8, 2024

Proposed Changes

  • 馃Ч Switch to Node 20 LTS

@openshift-ci openshift-ci bot requested review from jcrossley3 and rhuss March 8, 2024 13:39
@cardil cardil force-pushed the chore/node-20 branch 2 times, most recently from 0c9a194 to 2a59426 Compare March 8, 2024 21:04
@cardil
Copy link
Member Author

cardil commented Mar 8, 2024

/kind cleanup

Copy link
Contributor

openshift-ci bot commented Mar 8, 2024

@cardil: The label(s) kind/cleanup cannot be applied, because the repository doesn't have them.

In response to this:

/kind cleanup

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@matzew
Copy link
Member

matzew commented Mar 12, 2024

@matejvasek where do we generally stand w/ Node 20 ?
I think in upstream we also have older nodes?

@matzew
Copy link
Member

matzew commented Mar 12, 2024

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

@matejvasek where do we generally stand w/ Node 20 ? I think in upstream we also have older nodes?

At serverless-operator we just use node for testing. I think it makes 100% sense to follow the current supported Node version from func.

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

It turned out that the setup here, after we switched to RHEL8 as base for build image, was invalid: v10.24.0, see the build #1767238324688261120 @ L1886

Maybe we should switch to RHEL9, like what @mgencur is doing with openshift-knative/serving#650. Also, the OCP is using the RHEL9 for a while now. Then, we can do dnf install @nodejs:20, and install the RHEL provided node, instead of relying on nodesource.com' distribution. WDYT?

Something else isn't clear to me. I'm changing the Dockerfiles in this PR. But it seems the originals are being used for the CI, not the ones in this PR, see the build-log. But, our openshift/release configuration says the project Dockerfile should be used, so I presume the current, pre-merged files, not after the merge. No?!

@mgencur
Copy link
Contributor

mgencur commented Mar 12, 2024

Maybe we should switch to RHEL9, like what @mgencur is doing with openshift-knative/serving#650.

We use RHEL8 because it's closer to what what is productized in the end. There we use RHEL8. But I think it would also work if we switched to RHEL9 base image in CI. I don't see a big problem with that.

But, our openshift/release configuration says the project Dockerfile should be used, so I presume the current, pre-merged files, not after the merge. No?!

Your changes will be used only after-merge. This is for security reasons - one should not be able to change the build process just by sending a PR. At least that's what I was told a few years back.

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

@mgencur: Your changes will be used only after-merge. This is for security reasons - one should not be able to change the build process just by sending a PR. At least that's what I was told a few years back.

Makes sense. But, that means there is no way to test the changes before the merge?

@mgencur
Copy link
Contributor

mgencur commented Mar 12, 2024

Makes sense. But, that means there is no way to test the changes before the merge?

You can possibly create your own fork, create CI setup for it, and then send a PR for openshift/release targeting your fork. Some "formal" checks on the PR will fail but it will be possible to run "rehearsal" jobs.

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

@mgencur: We use RHEL8 because it's closer to what what is productized in the end.

Yes, but this is my point. To switch the base to RHEL 9 everywhere: in CI, testing, and productization. As I said. The OCP is being based on RHEL 9 for a couple of releases now, see: https://docs.openshift.com/container-platform/4.13/release_notes/ocp-4-13-release-notes.html#ocp-4-13-about-this-release

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

@mgencur: You can possibly create your own fork, create CI setup for it, and then send a PR for openshift/release targeting your fork. Some "formal" checks on the PR will fail but it will be possible to run "rehearsal" jobs.

That can be done in one PR? The "create CI setup for it, and then send a PR for openshift/release targeting your fork"?

If so, this is the same security thread, as running Dockerfiles from a PR. Most CI's allow to run a modified CI configuration, if it was posted by an approved contributor. Here it is even stranger, as the OpenShift CI/Prow supports the /ok-to-test command for newcommers.

@mgencur
Copy link
Contributor

mgencur commented Mar 12, 2024

That can be done in one PR? The "create CI setup for it, and then send a PR for openshift/release targeting your fork"?

I did it this way in the past - create my fork, include the required changes already on the given branch, and then send an "empty" PR against openshift/release against my fork to run rehearsal jobs. For the other concerns, please talk to DPTP.

@cardil
Copy link
Member Author

cardil commented Mar 12, 2024

@mgencur I did the exact test you proposed. Here you can see the successful job: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_release/49784/rehearse-49784-periodic-ci-cardil-serverless-operator-node20-main-414-ui-e2e-aws-414-c/1767657276690141184

@mgencur
Copy link
Contributor

mgencur commented Mar 13, 2024

/lgtm

Copy link
Contributor

openshift-ci bot commented Mar 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, mgencur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented Mar 13, 2024

@cardil: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/412-ui-e2e-aws-412 bdac232 link false /test 412-ui-e2e-aws-412
ci/prow/414-ui-e2e-aws-414 bdac232 link false /test 414-ui-e2e-aws-414
ci/prow/415-ui-e2e-aws-415 bdac232 link false /test 415-ui-e2e-aws-415
ci/prow/414-upstream-e2e-kafka-aws-414 bdac232 link false /test 414-upstream-e2e-kafka-aws-414

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@cardil
Copy link
Member Author

cardil commented Mar 13, 2024

/test 414-test-upgrade-aws-414

Looks like infra to me. Either way. This isn't connected.

@openshift-merge-bot openshift-merge-bot bot merged commit 680bf7b into openshift-knative:main Mar 13, 2024
19 of 23 checks passed
@cardil cardil deleted the chore/node-20 branch March 13, 2024 15:46
@cardil
Copy link
Member Author

cardil commented Mar 13, 2024

/cherry-pick release-1.32

@openshift-cherrypick-robot
Copy link
Contributor

@cardil: new pull request created: #2564

In response to this:

/cherry-pick release-1.32

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

Successfully merging this pull request may close these issues.

None yet

4 participants