chore: update node builder version

[matejvasek]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.44%. Comparing base (b97d841) to head (659b807).
Report is 25 commits behind head on main.

❗ Current head 659b807 differs from pull request most recent head 78964ac. Consider uploading reports for the commit 78964ac to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
- Coverage   64.21%   56.44%   -7.77%     
==========================================
  Files         108      108              
  Lines       13918    13919       +1     
==========================================
- Hits         8937     7857    -1080     
- Misses       4108     5279    +1171     
+ Partials      873      783      -90     

Flag	Coverage Δ
e2e-test	?
e2e-test-oncluster	30.50% <ø> (-0.01%)	⬇️
e2e-test-runtime-go	25.54% <ø> (?)
e2e-test-runtime-python	26.57% <ø> (?)
e2e-test-runtime-quarkus	26.68% <ø> (?)
e2e-test-runtime-rust	25.57% <ø> (?)
e2e-test-runtime-springboot	25.63% <ø> (?)
integration-tests	?
unit-tests-macos-latest	48.85% <ø> (+<0.01%)	⬆️
unit-tests-ubuntu-latest	49.59% <ø> (+<0.01%)	⬆️
unit-tests-windows-latest	48.89% <ø> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gauron99]

/lgtm

[jrangelramos]

/lgtm

[matejvasek]

/retest

[matejvasek]

PTAL @lkingland @gauron99

[lkingland]

/lgtm
/approve
/hold for flaky tests

[knative-prow]

New changes are detected. LGTM label has been removed.

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

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:

• OWNERS [lkingland,matejvasek]

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

[matejvasek]

Hmm these are not just flakes. Image build on newer image does not handle signals properly. We must inquiry why.

[matejvasek]

There is a bug in npm that is in nodejs-18 image. It does not propagate signals to child processes.

[matejvasek]

Node team is looking into this.

[dsimansk]

Rebased to rerun the CI on current version.

[dsimansk]

Is it changing the output of si2 build right?

[lkingland]

The build config's cfg.Tag should be set to f.Image if it is defined (this indicates a request to build exactly this image using, for example --image= flag via the CLI. If that is not defined, it should use f.Build.Image (the last built image) if it exists, which is essentially rebuilding. It seems like there might be a little missing logic here to choose one or the other? Am I seeing this correctly @gauron99 ?

[dsimansk]

Thanks for the explanation!
So I'd say it's a change unrelated to image version update then. Probably easier to address in an isolated issue/PR? Unless it was trying to solve something else during testing with NodeJS 18 image @matejvasek?

[gauron99]

client builder should have set the f.Build.Image to whatever the final resolved image is supposed to be (using forced image via f.Image or resolved via registry name etc.) like this , so Im pretty sure the f.Build.Image is correct here. It should be unrelated to node completely

[dsimansk]

There's a error in E2E tests even in unrelated runtimes, e.g. python. That seems like a genuine missing namespace value:

    cmd.go:128: exit status 1
    knative.go:35: services.serving.knative.dev "http-function-python-s2i" not found
    cmd.go:83: /home/runner/work/func/func/func delete --path /tmp/TestFunctionHttpTemplatepython_s2i_test187929058/001/http-function-python-s2i
    cmd.go:127: Error: namespace required

[lkingland]

Same comment here, I presumed we'd have a check early in the function which uses f.Image if defined, defaulting to f.Build.Image, with an error if neither are defined.

[gauron99]

There's a error in E2E tests even in unrelated runtimes, e.g. python. That seems like a genuine missing namespace value:

    knative.go:35: services.serving.knative.dev "http-function-python-s2i" not found
    cmd.go:83: /home/runner/work/func/func/func delete --path /tmp/TestFunctionHttpTemplatepython_s2i_test187929058/001/http-function-python-s2i
    cmd.go:127: Error: namespace required

I think there is an error during deploy actually, previous to this error there is Error: Error response from daemon: No such image: localhost:50000/user/cloudevents-function-python-s2i:latest during the pushing phase looks like, therefore the function is not deployed and namespace not defined during removal (nothing no remove anyways)

probably just related to the f.Build.Image change higher up since it looks like its affecting the s2i builds only

[matejvasek]

How that image thingy even got here? I don't know what's right but IMO it belongs to separate PR.

[dsimansk]

How that image thingy even got here? I don't know what's right but IMO it belongs to separate PR.

Might be my not very well done rebase then?

[lkingland]

How that image thingy even got here? I don't know what's right but IMO it belongs to separate PR.

Might be my not very well done rebase then?

It's very possible this is just a change which sneaked through a rebase, yes, since those lines were recently updated by @gauron99 to refer to f.Build.Image. I think those two changes could be removed, and that might clear up the E2E.