fix: force container=true for pack/s2i builders when not explicitly set #2966

RayyanSeliya · 2025-08-04T19:50:58Z

Added logic to automatically set container=true for containerized builders (pack/s2i) when --container flag is not explicitly provided by user
Added smart auto-selection to choose host builder when --container=false is specified without explicit builder
Fixes issue where func run --builder=pack would incorrectly run on host instead of container
Solution checks if builder is pack/s2i and --container flag wasn't changed by user
Resolves bug: function runs on host even when specified --builder #2955

Changes

🐛 Fix pack/s2i builders incorrectly running on host instead of container
✨ Add smart auto-selection for --container=false to choose host builder
🧪 Add comprehensive unit tests (TestCtrBuilder, TestCtrBuilderConflict, TestSmartBuilderSelection)
Add automatic container=true enforcement for containerized builders when flag not explicitly set
Preserve user choice when --container flag is explicitly provided

/kind bug

Release Note

Enhanced builder/container flag handling: pack/s2i builders now default to container mode, --container=false automatically selects host builder, with validation for incompatible combinations.

- Added logic to automatically set container=true for containerized builders (pack/s2i) when --container flag is not explicitly provided by user - Fixes issue where 'func run --builder=pack' would incorrectly run on host instead of container - Solution checks if builder is pack/s2i and --container flag wasn't changed by user - Resolves knative#2955

knative-prow · 2025-08-04T19:51:08Z

Hi @RayyanSeliya. 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. Regular contributors should join the org to skip this step.

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.

codecov · 2025-08-04T19:57:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.69%. Comparing base (d04ff0a) to head (f5b53a7).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2966      +/-   ##
==========================================
+ Coverage   57.55%   59.69%   +2.14%     
==========================================
  Files         132      132              
  Lines       16836    16944     +108     
==========================================
+ Hits         9690    10115     +425     
+ Misses       6237     5871     -366     
- Partials      909      958      +49

Flag	Coverage Δ
e2e-tests	`40.99% <25.00%> (+1.10%)`	⬆️
integration-tests	`54.48% <100.00%> (?)`
unit-tests	`46.64% <100.00%> (-0.25%)`	⬇️

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 · 2025-08-05T04:44:08Z

@RayyanSeliya Hi! thanks for the PR, much appreciated. I think this is a good addition, however Im not sure it fixes the issue completely (Thats my mistake for not mentioning it clearly in the issue). There is also another case, see below:
Running
❯ func run --builder=pack --container=false
tries to build with a host builder, which instead I think should error for an impossible match of flags because both builder and container were explicitly given.

cmd/run.go

@gauron99

- Auto-set container=true for pack/s2i when --container not explicitly set - Validate and error on incompatible --builder=pack --container=false combinations - Handle both flags and FUNC_CONTAINER environment variable Fixes knative#2955 Addresses @gauron99 feedback on explicit container=false validation

RayyanSeliya · 2025-08-05T10:56:46Z

@RayyanSeliya Hi! thanks for the PR, much appreciated. I think this is a good addition, however Im not sure it fixes the issue completely (Thats my mistake for not mentioning it clearly in the issue). There is also another case, see below: Running ❯ func run --builder=pack --container=false tries to build with a host builder, which instead I think should error for an impossible match of flags because both builder and container were explicitly given.

most welcome @gauron99 no worries for not mentioning i have inculcated the changes in the new commit !! thx

RayyanSeliya · 2025-08-05T11:01:24Z

hey @gauron99 i have also tested all the possibilities and tested can see the output here little bit messy !

rayyan@rayyan-seliya:/mnt/c/Users/RAYYAN/Desktop/func/test-fix$ 
# TEST 1: Original bug - should work now (force container=true)
echo "🧪 TEST 1: Pack builder auto-forces container mode"
../func-fixed run --builder=pack --build --registry=localhost:5000
# Expected:  Container operations (Docker pulls, etc.)


# TEST 2: Explicit container=false should ERROR  
echo "🧪 TEST 2: Pack + explicit container=false should error"
../func-fixed run --builder=pack --container=false --build --registry=localhost:5000
# Expected:  Error: "builder "pack" requires container mode but --container=false was explicitly set"

# TEST 3: Environment variable should ERROR
echo "🧪 TEST 3: FUNC_CONTAINER=false should error" 
FUNC_CONTAINER=false ../func-fixed run --builder=pack --build --registry=localhost:5000
# Expected:  Same error as above


# TEST 4: Host builder allows container=false (control test)
echo "🧪 TEST 4: Host builder allows container=false"
../func-fixed run --builder=host --container=false
# Expected:  Container operationscontainer=true --build --registry=localhost:5000


🧪 TEST 1: Pack builder auto-forces container mode
Building function image
Still building
Still building
^C^C
🧪 TEST 2: Pack + explicit container=false should error
Error: builder "pack" requires container mode but --container=false was explicitly set

🧪 TEST 3: FUNC_CONTAINER=false should error
Error: builder "pack" requires container mode but --container=false was explicitly set

🧪 TEST 4: Host builder allows container=false
Error: exec: "python": executable file not found in $PATH

🧪 TEST 5: Pack + explicit container=true works
Building function image
Still building

…nd also fixed some formatting issues to prevent test failure

gauron99

Thanks for the quick followup! I appreciate your time on this. I have a suggestion for a nicer solution using the Viper package (which takes care of the value fetching!) and you get to work with the final value already.

I suspect that a simple missmatch "if" in the
func (c runConfig) Validate(cmd *cobra.Command, f fn.Function) (err error) { function should be sufficient -- checking the container and builder together, making sure that these 2 values are compatible. I see that we have a check for a runtime with a container, this should be very similar

cmd/run.go

matejvasek · 2025-08-05T15:23:48Z

@RayyanSeliya I would add tests:

// in cmd/run_test.go
func TestCtrBuilder(t *testing.T) {
	var runner = mock.NewRunner()
	var builder = mock.NewBuilder()
	tmp := t.TempDir()

	fnPath := filepath.Join(tmp, "fn")
	f, err := fn.New().Init(fn.Function{Root: fnPath, Runtime: "python"})
	if err != nil {
		t.Fatal(err)
	}
	err = f.Write()
	if err != nil {
		t.Fatal(err)
	}
	t.Chdir(fnPath)

	cmd := NewRunCmd(NewTestClient(
		fn.WithRunner(runner),
		fn.WithBuilder(builder),
		fn.WithRegistry(TestRegistry),
	))
	cmd.SetArgs([]string{"--builder=pack", "--build=1"})
	ctx, cancel := context.WithCancel(context.Background())
	time.AfterFunc(time.Second, func() {
		cancel()
	})
	err = cmd.ExecuteContext(ctx)
	if err != nil {
		t.Fatal(err)
	}
	// verify that builder was called since `--builder=pack` was provided
	if !builder.BuildInvoked {
		t.Error("builder has not been invoked")
	}
}

func TestCtrBuilderConflict(t *testing.T) {
	var runner = mock.NewRunner()
	var builder = mock.NewBuilder()
	tmp := t.TempDir()

	fnPath := filepath.Join(tmp, "fn")
	f, err := fn.New().Init(fn.Function{Root: fnPath, Runtime: "python"})
	if err != nil {
		t.Fatal(err)
	}
	err = f.Write()
	if err != nil {
		t.Fatal(err)
	}
	t.Chdir(fnPath)

	cmd := NewRunCmd(NewTestClient(
		fn.WithRunner(runner),
		fn.WithBuilder(builder),
		fn.WithRegistry(TestRegistry),
	))
	cmd.SetArgs([]string{"--builder=pack", "--build=1", "--container=0"})
	ctx, cancel := context.WithCancel(context.Background())
	time.AfterFunc(time.Second, func() {
		cancel()
	})
	err = cmd.ExecuteContext(ctx)
        // since explicit flag `--builder=pack` and `--container=0` are contradiction we want error
	if err == nil {
		t.Error("error expected but got <nil>")
	}
}

matejvasek · 2025-08-05T15:28:29Z

test/e2e/scenario_no_container_test.go


 	knFuncTerm1 := common.NewKnFuncShellCli(t)
-	knFuncTerm1.Exec("create", "--language", "go", "--template", "http", funcPath)
+	knFuncTerm1.Exec("create", "--language", "go", "--template", "http", "--builder", "host", funcPath)


Shouldn't be host builder default here? We should not explicitly set it IMO.
wdyt @lkingland @gauron99

yeah @matejvasek! ,
You are absolutely right ,forcing users to explicitly specify --builder host when they want --container=false feels clunky. I think we could make this much more intuitive by having the CLI auto-select the appropriate builder based on the container flag.
The idea would be:> when someone runs func run --container=false, the system automatically picks host builder since it is the only one that makes sense for non-containerized runs. We would still validate and error out for incompatible combinations like --container=false --builder=pack, but the happy path would just work without users needing to know the "magic combination".
This approach would also make the E2E test work naturally without requiring the explicit --builder host flag , the system would be smart enough to choose the right builder automatically.
what do you think? worth implementing this smarter UX logic, or should we stick with the explicit approach? @lkingland @gauron99 would love your thoughts on this as well!

I would go the other way around, silencing the container flag more in favor of builder. I wasnt going to mention this because its beyond the scope of the issue but since you mentioned this, if you're up for it, you can even probably get rid of the container flag all together 😄 Having the builder would determine if it is a containerized or non-containerized build and you really dont need the container, hence now it really just adds the complexity of 2 flags needing to cooperate.

Since the host builder is always container-less and pack/s2i are always containerized, you determine the containerization via the builder flag alone. And you remove complexity which is a +

If you're not up for this, no worries, lets just get this PR finished for the bug and Im gonna tackle that later. In this case let's favor Matej's approach of keeping the host builder default then - Im guessing all you can do in this case is check the flag override ( what you did initially? 😄 )

Hey @gauron99!
I went ahead and implemented the smart auto-selection approach , now func run --container=false automatically picks the host builder, and func run --builder=pack forces container mode. The E2E test works naturally without needing explicit flags.
I love your idea about removing the container flag entirely and just using builder to determine everything , that would definitely be cleaner, For now this fixes the bug and improves the UX, but for the bigger refactor we can tackle it later i guess

… also added the test as requested to do so

RayyanSeliya · 2025-08-05T20:07:55Z

@RayyanSeliya I would add tests:

// in cmd/run_test.go
func TestCtrBuilder(t *testing.T) {
	var runner = mock.NewRunner()
	var builder = mock.NewBuilder()
	tmp := t.TempDir()

	fnPath := filepath.Join(tmp, "fn")
	f, err := fn.New().Init(fn.Function{Root: fnPath, Runtime: "python"})
	if err != nil {
		t.Fatal(err)
	}
	err = f.Write()
	if err != nil {
		t.Fatal(err)
	}
	t.Chdir(fnPath)

	cmd := NewRunCmd(NewTestClient(
		fn.WithRunner(runner),
		fn.WithBuilder(builder),
		fn.WithRegistry(TestRegistry),
	))
	cmd.SetArgs([]string{"--builder=pack", "--build=1"})
	ctx, cancel := context.WithCancel(context.Background())
	time.AfterFunc(time.Second, func() {
		cancel()
	})
	err = cmd.ExecuteContext(ctx)
	if err != nil {
		t.Fatal(err)
	}
	// verify that builder was called since `--builder=pack` was provided
	if !builder.BuildInvoked {
		t.Error("builder has not been invoked")
	}
}

func TestCtrBuilderConflict(t *testing.T) {
	var runner = mock.NewRunner()
	var builder = mock.NewBuilder()
	tmp := t.TempDir()

	fnPath := filepath.Join(tmp, "fn")
	f, err := fn.New().Init(fn.Function{Root: fnPath, Runtime: "python"})
	if err != nil {
		t.Fatal(err)
	}
	err = f.Write()
	if err != nil {
		t.Fatal(err)
	}
	t.Chdir(fnPath)

	cmd := NewRunCmd(NewTestClient(
		fn.WithRunner(runner),
		fn.WithBuilder(builder),
		fn.WithRegistry(TestRegistry),
	))
	cmd.SetArgs([]string{"--builder=pack", "--build=1", "--container=0"})
	ctx, cancel := context.WithCancel(context.Background())
	time.AfterFunc(time.Second, func() {
		cancel()
	})
	err = cmd.ExecuteContext(ctx)
        // since explicit flag `--builder=pack` and `--container=0` are contradiction we want error
	if err == nil {
		t.Error("error expected but got <nil>")
	}
}

thx @matejvasek for this test suggestion i have added this in my recent commit !!

automatically handles pack->container=true and container=false->host builder. also updated the e2e test and added new smartbuilderselection test in run_test.go

gauron99

thanks for the contribution! This looks good to me
/lgtm
/approve
/hold for Matej's review

knative-prow · 2025-08-07T12:23:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

matejvasek · 2025-08-07T12:45:53Z

/unhold

RayyanSeliya · 2025-08-07T12:46:55Z

thanks for the contribution! This looks good to me
/lgtm
/approve
/hold for Matej's review

Thx @gauron99 &@matejvasek for being patience throughout the pr !!

knative-prow bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 4, 2025

knative-prow bot requested review from nainaz and vyasgun August 4, 2025 19:51

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 4, 2025

gauron99 requested review from gauron99 and removed request for nainaz and vyasgun August 5, 2025 04:47

gauron99 reviewed Aug 5, 2025

View reviewed changes

cmd/run.go Outdated Show resolved Hide resolved

gauron99 removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2025

gauron99 self-assigned this Aug 5, 2025

knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2025

added logic to prevent test failure in testfunctionwithoutcontainer a…

6f37188

…nd also fixed some formatting issues to prevent test failure

gauron99 reviewed Aug 5, 2025

View reviewed changes

cmd/run.go Outdated Show resolved Hide resolved

matejvasek reviewed Aug 5, 2025

View reviewed changes

refactored the way of implementation based on the suggestion provided…

0ac78a8

… also added the test as requested to do so

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2025

fixed the linting errors

4b2b46c

RayyanSeliya requested a review from matejvasek August 5, 2025 20:48

RayyanSeliya requested a review from gauron99 August 5, 2025 20:49

fimplemented smart builder/container auto-selection

6e5b429

automatically handles pack->container=true and container=false->host builder. also updated the e2e test and added new smartbuilderselection test in run_test.go

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2025

fix linting errors

f5b53a7

gauron99 approved these changes Aug 7, 2025

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 7, 2025

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2025

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2025

knative-prow bot merged commit 065006c into knative:main Aug 7, 2025
39 checks passed

fix: force container=true for pack/s2i builders when not explicitly set #2966

fix: force container=true for pack/s2i builders when not explicitly set #2966

Uh oh!

Conversation

RayyanSeliya commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

knative-prow bot commented Aug 4, 2025

Uh oh!

codecov bot commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gauron99 commented Aug 5, 2025

Uh oh!

Uh oh!

RayyanSeliya commented Aug 5, 2025

Uh oh!

RayyanSeliya commented Aug 5, 2025

Uh oh!

gauron99 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matejvasek commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejvasek Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

RayyanSeliya Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauron99 Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RayyanSeliya Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

RayyanSeliya commented Aug 5, 2025

Uh oh!

gauron99 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Aug 7, 2025

Uh oh!

matejvasek commented Aug 7, 2025

Uh oh!

Uh oh!

RayyanSeliya commented Aug 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RayyanSeliya commented Aug 4, 2025 •

edited

Loading

codecov bot commented Aug 4, 2025 •

edited

Loading

gauron99 left a comment •

edited

Loading

matejvasek commented Aug 5, 2025 •

edited

Loading

RayyanSeliya Aug 5, 2025 •

edited

Loading

gauron99 Aug 6, 2025 •

edited

Loading