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/s2i that detects the func-generated legacy .s2i/bin/assemble file and prints a warning to stderr
🧹 Wire the warning into both the local Build() path (pkg/s2i/builder.go) and the remote pipeline Run() path (pkg/pipelines/tekton/pipelines_provider.go) so it fires regardless of how the function is built or deployed
🎁 Add table-driven unit tests covering: scaffolded runtime with legacy file (warns), non-scaffolded runtime with legacy file (silent), scaffolded runtime without legacy file (silent)

/kind enhancement

Release Note

When building or deploying a go/python function via the S2I builder, func now warns if a legacy `.s2i/bin/assemble` file is present at the function root. This file was generated by older versions of func and has been superseded by `.func/build/bin/assemble`. The stale file can cause unexpected behaviour with other builders (e.g. pack erroneously detecting the assemble script). The warning is emitted for both local builds and remote pipeline deployments.

Docs

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:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Ankitsinghsisodya
Once this PR has been reviewed and has the lgtm label, please assign dsimansk for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

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

pkg/s2i/scaffolder.go

+	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

pkg/s2i/builder_test.go

+			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

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 56.30%. Comparing base (3e493ac) to head (d2496ff).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/pipelines/tekton/pipelines_provider.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3584      +/-   ##
==========================================
+ Coverage   56.24%   56.30%   +0.05%     
==========================================
  Files         180      180              
  Lines       20465    20528      +63     
==========================================
+ Hits        11511    11558      +47     
- Misses       7755     7765      +10     
- Partials     1199     1205       +6

Flag	Coverage Δ
e2e	`36.13% <47.61%> (+0.02%)`	⬆️
e2e go	`32.57% <33.33%> (+0.02%)`	⬆️
e2e node	`28.32% <41.66%> (-0.04%)`	⬇️
e2e python	`32.95% <33.33%> (+0.03%)`	⬆️
e2e quarkus	`28.45% <41.66%> (-0.06%)`	⬇️
e2e rust	`27.89% <41.66%> (-0.04%)`	⬇️
e2e springboot	`26.33% <41.66%> (-0.05%)`	⬇️
e2e typescript	`28.43% <41.66%> (-0.04%)`	⬇️
e2e-config-ci	`18.08% <0.00%> (-0.07%)`	⬇️
integration	`17.41% <33.33%> (-0.02%)`	⬇️
unit macos-14	`43.40% <91.66%> (+0.03%)`	⬆️
unit macos-latest	`43.40% <91.66%> (+0.03%)`	⬆️
unit ubuntu-24.04-arm	`43.59% <90.47%> (+0.02%)`	⬆️
unit ubuntu-latest	`44.28% <91.66%> (+0.02%)`	⬆️
unit windows-latest	`43.43% <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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when legacy .s2i/bin/assemble scaffolding is detected#3584

Warn when legacy .s2i/bin/assemble scaffolding is detected#3584
Ankitsinghsisodya wants to merge 2 commits intoknative:mainfrom
Ankitsinghsisodya:issue-3452-legacy-s2i-warning

Ankitsinghsisodya commented Apr 5, 2026

Uh oh!

knative-prow bot commented Apr 5, 2026

Uh oh!

knative-prow bot commented Apr 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

Copilot AI Apr 5, 2026

Uh oh!

codecov bot commented Apr 5, 2026 •

edited

Loading

Uh oh!

gauron99 commented Apr 8, 2026

Uh oh!

Ankitsinghsisodya commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Conversation

Ankitsinghsisodya commented Apr 5, 2026

Changes

Uh oh!

knative-prow bot commented Apr 5, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Apr 5, 2026 •

edited

Loading