Warn when legacy .s2i/bin/assemble scaffolding is detected by Ankitsinghsisodya · Pull Request #3584 · knative/func

Ankitsinghsisodya · 2026-04-05T10:06:37Z

Changes

Add WarnIfLegacyS2IScaffolding helper in pkg/functions that detects the legacy .s2i/bin/assemble
file for scaffolded runtimes (go/python) and prints a warning to stderr
Call it from Client.Build (covers all local builds/deploys) and PipelinesProvider.Run (covers
remote deploys)
Add table-driven unit tests
/kind enhancement

Release Note

Warn during build when legacy path func-root/.s2i/bin/assemble is still present. With new scaffolding this is moved under unified .func/ hidden directory.

Prior to knative#3436, func wrote S2I scaffolding for go/python to <func-root>/.s2i/bin/assemble. It now lives at .func/build/bin/assemble. The stale file can interfere with other builders (e.g. pack erroneously detects the assemble script). Add WarnIfLegacyS2IScaffolding helper in pkg/s2i that checks only scaffolded runtimes (go/python) and only the exact file func generated, so user-managed .s2i directories are not flagged. Call it from both the local Build() path and the remote PipelinesProvider.Run() path so the warning is emitted regardless of how the function is built. Fixes knative#3452

knative-prow · 2026-04-05T10:06:48Z

Hi @Ankitsinghsisodya. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

Copilot

Pull request overview

Adds a centralized warning for legacy func-generated S2I scaffolding (<func-root>/.s2i/bin/assemble) so users are alerted when stale files may interfere with current builds/deployments after the scaffolding move to .func/build.

Changes:

Added WarnIfLegacyS2IScaffolding helper in pkg/s2i to detect the legacy .s2i/bin/assemble and emit a warning.
Wired the warning into both local S2I builds (Builder.Build) and remote Tekton pipeline runs (PipelinesProvider.Run).
Added table-driven unit coverage to ensure warning behavior matches scaffolded vs non-scaffolded runtimes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pkg/s2i/scaffolder.go	Introduces the legacy scaffolding detection + warning helper.
pkg/s2i/builder.go	Invokes the warning during local S2I builds.
pkg/s2i/builder_test.go	Adds tests validating warning emission/suppression behavior.
pkg/pipelines/tekton/pipelines_provider.go	Invokes the warning during remote pipeline runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-05T10:09:35Z

+	legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble")
+	if _, err := os.Stat(legacyAssemble); err == nil {


The docstring says this check is scoped to “the exact file func generated” so user-managed .s2i dirs aren’t flagged, but the implementation only checks for existence of .s2i/bin/assemble. This will warn even if the user intentionally created their own assemble script. Either update the comment to match the behavior, or (preferably) read the file and verify it matches the known legacy func-generated script content (e.g., compare against GoAssembler/PythonAssembler) before warning.

Suggested change

legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble")

if _, err := os.Stat(legacyAssemble); err == nil {

expectedAssemble, err := assembler(f)

if err != nil || expectedAssemble == "" {

return

}

legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble")

content, err := os.ReadFile(legacyAssemble)

if err != nil {

return

}

if string(content) == expectedAssemble {

Copilot · 2026-04-05T10:09:35Z

+			oldStderr := os.Stderr
+			r, w, err := os.Pipe()
+			if err != nil {
+				t.Fatal(err)
+			}
+			os.Stderr = w
+
+			i := &mockImpl{BuildFn: func(cfg *api.Config) (*api.Result, error) { return nil, nil }}
+			b := s2i.NewBuilder(s2i.WithImpl(i), s2i.WithDockerClient(mockDocker{}))
+			buildErr := b.Build(t.Context(), f, nil)
+
+			w.Close()
+			os.Stderr = oldStderr
+
+			var buf bytes.Buffer
+			if _, err := io.Copy(&buf, r); err != nil {
+				t.Fatal(err)


This test redirects the process-global os.Stderr but doesn’t use t.Cleanup/defer to guarantee restoration (and pipe closure) on early failures/panics. It also never closes the read end of the pipe, which can leak FDs across the test suite. Use t.Cleanup to restore os.Stderr and close both pipe ends, and avoid leaving global state changed if the test fails mid-way.

codecov · 2026-04-05T10:14:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.29%. Comparing base (3e493ac) to head (b2a1ebb).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3584      +/-   ##
==========================================
+ Coverage   56.24%   56.29%   +0.04%     
==========================================
  Files         180      180              
  Lines       20465    20543      +78     
==========================================
+ Hits        11511    11564      +53     
- Misses       7755     7778      +23     
- Partials     1199     1201       +2

Flag	Coverage Δ
e2e	`36.20% <47.05%> (+0.10%)`	⬆️
e2e go	`32.85% <41.66%> (+0.29%)`	⬆️
e2e node	`28.60% <50.00%> (+0.24%)`	⬆️
e2e python	`33.22% <41.66%> (+0.30%)`	⬆️
e2e quarkus	`28.74% <50.00%> (+0.23%)`	⬆️
e2e rust	`28.17% <50.00%> (+0.24%)`	⬆️
e2e springboot	`26.61% <50.00%> (+0.22%)`	⬆️
e2e typescript	`28.71% <50.00%> (+0.23%)`	⬆️
e2e-config-ci	`18.06% <0.00%> (-0.09%)`	⬇️
integration	`17.48% <41.66%> (+0.05%)`	⬆️
unit macos-14	`43.34% <91.66%> (-0.04%)`	⬇️
unit macos-latest	`43.34% <91.66%> (-0.04%)`	⬇️
unit ubuntu-24.04-arm	`43.53% <94.11%> (-0.04%)`	⬇️
unit ubuntu-latest	`44.21% <91.66%> (-0.04%)`	⬇️
unit windows-latest	`43.36% <91.66%> (-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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gauron99 · 2026-04-08T08:57:40Z

I think we could actually run the warning on every deployment, because user mightve used s2i, then switched to a different builder. This would persist the .s2i/ directory in the function. Granted there was a bug where running w/ s2i and subsequently running w/ pack a different, cryptic error would occur becuase of how pack works. (this was the side-effect).

Either every deploy or every build (deploy uses build under the hood, that is the client function client.Build() -- so probably client.Build should call this warn)

Remote deployment keeps the call but would not have to import the s2i when the function is moved (see below)

Scoping only to scaffolded runtimes (go/python) looks correct to me.

The warn function itself could live in the pkg/functions/ as a standalone function.

Per review feedback on knative#3584, the warning helper now lives in pkg/functions as a standalone WarnIfLegacyS2IScaffolding and is invoked from Client.Build, so it fires for every build (and therefore every local deploy) regardless of which builder the user switched to. The Tekton pipelines provider keeps its own call for remote deploys (which don't go through Client.Build) and no longer needs to import pkg/s2i. The s2i builder's redundant call is removed. The test moves to pkg/functions and exercises the helper directly.

Ankitsinghsisodya · 2026-04-10T03:12:13Z

Thanks for the review @gauron99 sir!

Pushed an update that does exactly this:

Moved the warn into pkg/functions as a standalone WarnIfLegacyS2IScaffolding
Hooked it into Client.Build so it runs on every build (and therefore every local deploy, since deploy goes through build under the hood) regardless of which builder is active
Dropped the duplicated call from the s2i builder
Tekton's PipelinesProvider.Run keeps its own call for the remote-deploy path (which doesn't go through Client.Build) and no longer needs to import pkg/s2i
Test moved to pkg/functions and now exercises the helper directly instead of going through a full builder run

Kept the HasScaffolding() gate so it stays scoped to go/python.

Ankitsinghsisodya · 2026-04-13T20:50:40Z

@gauron99 @matejvasek Can you look into this PR?

gauron99

Thanks for the updates, looking much cleaner :) just few notes from me

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Ankitsinghsisodya · 2026-04-14T14:03:35Z

@gauron99
in source-to-image v1.5.1, pkg/scripts/install.go around line 204, handlers get registered in this order and the first one that has a given script wins:

ScriptsURL (if set)
.s2i/bin/ from the source tree
the image's scripts-url label

we set ScriptsURL to file:///.func/build/bin for go/python in pkg/s2i/builder.go:184, so s2i grabs assemble from there and never even looks at .s2i/bin/assemble. for run and save-artifacts it would fall through to .s2i/bin/, but func only ever wrote assemble, so there's nothing legacy to pick up for those.

so the s2i builder itself is fine. where the stale file actually bites you is pack — it sees .s2i/bin/assemble and treats it as a signal. that's really what the warning is for. if "may interfere with other builders" sounds too vague i can call out pack specifically, lmk.

gauron99 · 2026-04-15T18:54:12Z

/ok-to-test

gauron99

hey, code looks good, found extra Fprintf without any % directive. Re-reading comments again, I thought of better wording :)

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

gauron99

oops one got left there

gauron99

/lgtm
Thanks a lot!

knative-prow · 2026-04-15T20:00:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitsinghsisodya, gauron99

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [gauron99]

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

Copilot AI review requested due to automatic review settings April 5, 2026 10:06

knative-prow bot added the kind/enhancement Feature additions or improvements to existing label Apr 5, 2026

knative-prow bot requested review from dsimansk and jrangelramos April 5, 2026 10:06

knative-prow bot added the size/L 🤖 PR changes 100-499 lines, ignoring generated files. label Apr 5, 2026

knative-prow bot added the needs-ok-to-test 🤖 Needs an org member to approve testing label Apr 5, 2026

Copilot started reviewing on behalf of Ankitsinghsisodya April 5, 2026 10:07 View session

Copilot AI reviewed Apr 5, 2026

View reviewed changes

knative-prow bot added size/M 🤖 PR changes 30-99 lines, ignoring generated files. and removed size/L 🤖 PR changes 100-499 lines, ignoring generated files. labels Apr 10, 2026

gauron99 reviewed Apr 14, 2026

View reviewed changes

Comment thread pkg/functions/client.go Outdated

Comment thread pkg/functions/function.go Outdated

Comment thread pkg/functions/function.go Outdated

Comment thread pkg/functions/function_unit_test.go Outdated

Ankitsinghsisodya and others added 3 commits April 14, 2026 13:32

Update pkg/functions/client.go

d42729d

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Update pkg/functions/function.go

89c73ee

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Update pkg/functions/function_unit_test.go

549d25b

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Fix govet/staticcheck: drop unused Fprintf args in legacy s2i warning

2692563

knative-prow bot added ok-to-test 🤖 Non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test 🤖 Needs an org member to approve testing labels Apr 15, 2026

gauron99 reviewed Apr 15, 2026

View reviewed changes

Comment thread pkg/functions/function.go Outdated

Comment thread pkg/functions/function.go Outdated

Comment thread pkg/functions/function.go Outdated

Ankitsinghsisodya and others added 3 commits April 16, 2026 01:08

Update pkg/functions/function.go

e988f68

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Update pkg/functions/function.go

6ba9d62

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

Update pkg/functions/function.go

8681cda

Co-authored-by: David Fridrich <49119790+gauron99@users.noreply.github.com>

gauron99 reviewed Apr 15, 2026

View reviewed changes

Comment thread pkg/functions/function.go Outdated

Update pkg/functions/function.go

b2a1ebb

gauron99 approved these changes Apr 15, 2026

View reviewed changes

knative-prow bot assigned gauron99 Apr 15, 2026

knative-prow bot added the lgtm 🤖 PR is ready to be merged. label Apr 15, 2026

knative-prow bot added the approved 🤖 PR has been approved by an approver from all required OWNERS files. label Apr 15, 2026

knative-prow bot merged commit da85045 into knative:main Apr 15, 2026
44 checks passed

		legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble")
		if _, err := os.Stat(legacyAssemble); err == nil {

Conversation

Ankitsinghsisodya commented Apr 5, 2026 • edited by gauron99 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

knative-prow bot commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gauron99 commented Apr 8, 2026

Uh oh!

Ankitsinghsisodya commented Apr 10, 2026

Uh oh!

Ankitsinghsisodya commented Apr 13, 2026

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ankitsinghsisodya commented Apr 14, 2026

Uh oh!

gauron99 commented Apr 15, 2026

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Apr 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ankitsinghsisodya commented Apr 5, 2026 •

edited by gauron99

Loading

codecov bot commented Apr 5, 2026 •

edited

Loading