-
Notifications
You must be signed in to change notification settings - Fork 138
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
chore: update node builder version #2094
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/lgtm |
/lgtm |
/retest |
e5a4682
to
f0d3497
Compare
PTAL @lkingland @gauron99 |
/lgtm |
New changes are detected. LGTM label has been removed. |
[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:
Approvers can indicate their approval by writing |
8d07825
to
0928cc0
Compare
Hmm these are not just flakes. Image build on newer image does not handle signals properly. We must inquiry why. |
There is a bug in |
0928cc0
to
adb68e7
Compare
Node team is looking into this. |
Signed-off-by: Matej Vasek <mvasek@redhat.com>
Signed-off-by: Matej Vasek <mvasek@redhat.com>
659b807
to
520a8cf
Compare
Rebased to rerun the CI on current version. |
@@ -139,7 +139,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf | |||
// Build Config | |||
cfg := &api.Config{} | |||
cfg.Quiet = !b.verbose | |||
cfg.Tag = f.Build.Image | |||
cfg.Tag = f.Image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it changing the output of si2 build right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There's a error in E2E tests even in unrelated runtimes, e.g. python. That seems like a genuine missing
|
@@ -319,7 +319,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf | |||
}() | |||
|
|||
opts := types.ImageBuildOptions{ | |||
Tags: []string{f.Build.Image}, | |||
Tags: []string{f.Image}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I think there is an error during deploy actually, previous to this error there is probably just related to the |
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 |
No description provided.