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

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

- 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

* fix: enforce container mode for pack/s2i builders

- 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

* added logic to prevent test failure in testfunctionwithoutcontainer and also fixed some formatting issues to prevent test failure

* refactored the way of implementation based on the  suggestion provided also added the test as requested to do so

* fixed the linting errors

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

* fix linting errors

Author: RayyanSeliya

cmd/run.go

@@ -171,16 +171,31 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) {
 	if f, err = fn.NewFunction(cfg.Path); err != nil {
 		return
 	}
-	if err = cfg.Validate(cmd, f); err != nil {
-		return
-	}
 	if !f.Initialized() {
 		return fn.NewErrNotInitialized(f.Root)
 	}
 	if f, err = cfg.Configure(f); err != nil { // Updates f with deploy cfg
 		return
 	}
 
+	// Smart auto-fix logic for builder/container compatibility
+	// This fixes the original bug where --builder=pack doesn't default to container=true
+
+	// Case 1: Containerized builders (pack/s2i) should force container=true when not explicitly set
+	if (f.Build.Builder == "pack" || f.Build.Builder == "s2i") && !cfg.Container && !cmd.Flags().Changed("container") {
+		cfg.Container = true
+	}
+
+	// Case 2: container=false should auto-select host builder when no builder explicitly set
+	if !cfg.Container && cmd.Flags().Changed("container") && !cmd.Flags().Changed("builder") {
+		f.Build.Builder = "host"
+	}
+
+	// Validate after configure and auto-fix
+	if err = cfg.Validate(cmd, f); err != nil {
+		return
+	}
+
 	// Ignore the verbose flag if JSON output
 	if cfg.JSON {
 		cfg.Verbose = false
@@ -401,6 +416,11 @@ func (c runConfig) Validate(cmd *cobra.Command, f fn.Function) (err error) {
 		return fmt.Errorf("the %q runtime currently requires being run in a container", f.Runtime)
 	}
 
+	// Validate that containerized builders (pack/s2i) cannot be used with container=false
+	if (f.Build.Builder == "pack" || f.Build.Builder == "s2i") && !c.Container {
+		return fmt.Errorf("builder %q requires container mode but --container=false was set", f.Build.Builder)
+	}
+
 	// When the docker runner respects the StartTimeout, this validation check
 	// can be removed
 	if c.StartTimeout != 0 && c.Container {

cmd/run_test.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -601,3 +602,107 @@ func TestRun_BaseImage(t *testing.T) {
 		})
 	}
 }
+
+// TestCtrBuilder tests that pack builder auto-forces container mode
+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")
+	}
+}
+
+// TestCtrBuilderConflict tests that pack builder with explicit container=false shows validation error
+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>")
+	}
+}
+
+// TestSmartBuilderSelection tests that container=false auto-selects host builder
+func TestSmartBuilderSelection(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: "go"})
+	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{"--container=false", "--build=1"})
+	ctx, cancel := context.WithCancel(context.Background())
+	time.AfterFunc(time.Second, func() {
+		cancel()
+	})
+	err = cmd.ExecuteContext(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+}