From 07e92768c5ffb569335f8e2c0a4f62804d0f3f37 Mon Sep 17 00:00:00 2001 From: Prateek Singh Rathore Date: Wed, 16 Sep 2026 17:09:24 +0530 Subject: [PATCH 1/2] Add step-resources and buildrun-name flags to build upload `shp build upload` created its own BuildRun and exposed only a fixed set of BuildRun fields, so a Local-source build could not request step compute resources and could not be streamed into a BuildRun created elsewhere. This closes the gap flagged when migrating OpenShift BuildConfigs (with spec.resources) to Shipwright. - --step-resources (repeatable): sets BuildRunSpec.StepResources as =.=, e.g. build-and-push=limits.memory=2Gi. Wired through BuildRunSpecFromFlags, so it is also available on `build run` and `buildrun create`. - --buildrun-name: streams the local source into an existing BuildRun instead of creating one, covering any BuildRun field the CLI does not expose as a flag. Guarded against combination with creation-only flags and against source-bundle builds. Adds unit tests for the flag parser and upload flow, plus e2e cases, and regenerates the command docs. Closes: #415 Co-authored-by: Claude Signed-off-by: Prateek Singh Rathore --- docs/shp_build_run.md | 1 + docs/shp_build_upload.md | 2 + docs/shp_buildrun_create.md | 1 + pkg/shp/cmd/build/upload.go | 76 +++++++++++++- pkg/shp/cmd/build/upload_test.go | 110 +++++++++++++++++++++ pkg/shp/flags/buildrun.go | 1 + pkg/shp/flags/flags.go | 14 +++ pkg/shp/flags/step_resources_value.go | 90 +++++++++++++++++ pkg/shp/flags/step_resources_value_test.go | 83 ++++++++++++++++ test/e2e/upload.bats | 104 +++++++++++++++++++ 10 files changed, 480 insertions(+), 2 deletions(-) create mode 100644 pkg/shp/cmd/build/upload_test.go create mode 100644 pkg/shp/flags/step_resources_value.go create mode 100644 pkg/shp/flags/step_resources_value_test.go diff --git a/docs/shp_build_run.md b/docs/shp_build_run.md index 2ccff9eb1..72457c94b 100644 --- a/docs/shp_build_run.md +++ b/docs/shp_build_run.md @@ -34,6 +34,7 @@ shp build run [flags] --runtime-class string specify the runtime class to be used for the Pod --sa-name string Kubernetes service-account name --scheduler-name string specify the scheduler to be used to dispatch the Pod + --step-resources stringArray override compute resources for a build step, as =.= (repeatable) (default []) --timeout duration build process timeout ``` diff --git a/docs/shp_build_upload.md b/docs/shp_build_upload.md index 19fd2b13d..63bc1450a 100644 --- a/docs/shp_build_upload.md +++ b/docs/shp_build_upload.md @@ -29,6 +29,7 @@ shp build upload [path/to/source|.] [flags] ``` --buildref-name string name of build resource to reference + --buildrun-name string stream the local source into an existing BuildRun instead of creating one -e, --env stringArray specify a key-value pair for an environment variable to set for the build container (default []) -F, --follow Start a build and watch its log until it completes or fails. -h, --help help for upload @@ -44,6 +45,7 @@ shp build upload [path/to/source|.] [flags] --runtime-class string specify the runtime class to be used for the Pod --sa-name string Kubernetes service-account name --scheduler-name string specify the scheduler to be used to dispatch the Pod + --step-resources stringArray override compute resources for a build step, as =.= (repeatable) (default []) --timeout duration build process timeout ``` diff --git a/docs/shp_buildrun_create.md b/docs/shp_buildrun_create.md index 912f2f89d..6af963464 100644 --- a/docs/shp_buildrun_create.md +++ b/docs/shp_buildrun_create.md @@ -33,6 +33,7 @@ shp buildrun create [flags] --runtime-class string specify the runtime class to be used for the Pod --sa-name string Kubernetes service-account name --scheduler-name string specify the scheduler to be used to dispatch the Pod + --step-resources stringArray override compute resources for a build step, as =.= (repeatable) (default []) --timeout duration build process timeout ``` diff --git a/pkg/shp/cmd/build/upload.go b/pkg/shp/cmd/build/upload.go index f75bea9b1..5927022c8 100644 --- a/pkg/shp/cmd/build/upload.go +++ b/pkg/shp/cmd/build/upload.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path" + "strings" "time" buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1" @@ -17,6 +18,7 @@ import ( "github.com/shipwright-io/cli/pkg/shp/reactor" "github.com/shipwright-io/cli/pkg/shp/streamer" "github.com/spf13/cobra" + "github.com/spf13/pflag" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -30,6 +32,7 @@ type UploadCommand struct { follow bool // flag to tail pod logs buildRefName string // build name + buildRunName string // name of an existing BuildRun to stream into, instead of creating one sourceDir string // local directory to be streamed dataStreamer *streamer.Streamer // tar streamer instance @@ -152,6 +155,42 @@ func (u *UploadCommand) Validate() error { if !stat.IsDir() { return fmt.Errorf("informed path is not a directory: '%s'", u.sourceDir) } + if err := u.checkBuildRunNameConflicts(); err != nil { + return err + } + // streaming into an existing BuildRun only supports the local-copy path; a source bundle image + // requires this command to create the BuildRun and push the bundle itself. + if u.buildRunName != "" && u.sourceBundleImage != "" { + return fmt.Errorf("--%s is not supported for Builds using a source bundle image", flags.BuildrunNameFlag) + } + return nil +} + +// checkBuildRunNameConflicts makes sure that, when an existing BuildRun is referenced with the +// --buildrun-name flag, none of the flags that only apply to a BuildRun this command creates are +// also set, since those settings would be silently ignored. +func (u *UploadCommand) checkBuildRunNameConflicts() error { + if u.buildRunName == "" { + return nil + } + // flags that remain valid together with --buildrun-name + allowed := map[string]bool{ + flags.BuildrunNameFlag: true, + flags.BuildrefNameFlag: true, // set programmatically from the positional argument + "follow": true, + } + var conflicting []string + u.cmd.Flags().Visit(func(f *pflag.Flag) { + if !allowed[f.Name] { + conflicting = append(conflicting, "--"+f.Name) + } + }) + if len(conflicting) > 0 { + return fmt.Errorf( + "--%s cannot be combined with %s; set those fields on the BuildRun you create instead", + flags.BuildrunNameFlag, strings.Join(conflicting, ", "), + ) + } return nil } @@ -203,6 +242,28 @@ func (u *UploadCommand) createBuildRun(p *params.Params) (*buildv1beta1.BuildRun return br, nil } +// resolveBuildRun returns the BuildRun to stream into: when --buildrun-name is set it fetches the +// existing BuildRun, otherwise it creates a new one. +func (u *UploadCommand) resolveBuildRun(p *params.Params) (*buildv1beta1.BuildRun, error) { + if u.buildRunName == "" { + return u.createBuildRun(p) + } + + ns := p.Namespace() + clientset, err := p.ShipwrightClientSet() + if err != nil { + return nil, err + } + br, err := clientset.ShipwrightV1beta1(). + BuildRuns(ns). + Get(u.cmd.Context(), u.buildRunName, metav1.GetOptions{}) + if err != nil { + return nil, err + } + fmt.Fprintf(u.ioStreams.Out, "Streaming into existing BuildRun '%s/%s'...\n", ns, br.GetName()) + return br, nil +} + // performDataStreaming execute the data transfer process end-to-end. func (u *UploadCommand) performDataStreaming(target *streamer.Target) error { if u.streamingIsDone { @@ -282,8 +343,8 @@ func (u *UploadCommand) onPodModifiedEventBundling(pod *corev1.Pod) error { // Run executes the primary business logic of this subcommand, by starting to watch over the build // pod status and react accordingly. func (u *UploadCommand) Run(p *params.Params, ioStreams *genericclioptions.IOStreams) error { - // creating a BuildRun with settings for the local source upload - br, err := u.createBuildRun(p) + // creating a BuildRun with settings for the local source upload, or resolving an existing one + br, err := u.resolveBuildRun(p) if err != nil { return err } @@ -340,5 +401,16 @@ func uploadCmd() runner.SubCommand { follow: false, } flags.FollowFlag(cmd.Flags(), &u.follow) + buildRunNameFlag(cmd.Flags(), &u.buildRunName) return u } + +// buildRunNameFlag registers the --buildrun-name flag, storing the value on the informed pointer. +func buildRunNameFlag(flagSet *pflag.FlagSet, buildRunName *string) { + flagSet.StringVar( + buildRunName, + flags.BuildrunNameFlag, + "", + "stream the local source into an existing BuildRun instead of creating one", + ) +} diff --git a/pkg/shp/cmd/build/upload_test.go b/pkg/shp/cmd/build/upload_test.go new file mode 100644 index 000000000..97200d669 --- /dev/null +++ b/pkg/shp/cmd/build/upload_test.go @@ -0,0 +1,110 @@ +package build // nolint:revive + +import ( + "strings" + "testing" + + buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1" + shpfake "github.com/shipwright-io/build/pkg/client/clientset/versioned/fake" + "github.com/shipwright-io/cli/pkg/shp/flags" + "github.com/shipwright-io/cli/pkg/shp/params" + "github.com/spf13/cobra" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/kubernetes/fake" +) + +// newUploadCommandForTest builds an UploadCommand wired with a fresh cobra command and the standard +// BuildRun spec flags, mimicking uploadCmd(). +func newUploadCommandForTest() *UploadCommand { + ccmd := &cobra.Command{} + u := &UploadCommand{ + cmd: ccmd, + buildRunSpec: flags.BuildRunSpecFromFlags(ccmd.Flags()), + } + flags.FollowFlag(ccmd.Flags(), &u.follow) + buildRunNameFlag(ccmd.Flags(), &u.buildRunName) + return u +} + +func TestUploadValidateBuildRunNameConflicts(t *testing.T) { + t.Run("buildrun-name with a spec flag is rejected", func(t *testing.T) { + u := newUploadCommandForTest() + if err := u.cmd.Flags().Set(flags.BuildrunNameFlag, "existing-br"); err != nil { + t.Fatal(err) + } + if err := u.cmd.Flags().Set(flags.ServiceAccountNameFlag, "builder"); err != nil { + t.Fatal(err) + } + + err := u.checkBuildRunNameConflicts() + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), flags.ServiceAccountNameFlag) { + t.Errorf("error should mention the conflicting flag, got: %s", err.Error()) + } + }) + + t.Run("buildrun-name alone is accepted", func(t *testing.T) { + u := newUploadCommandForTest() + if err := u.cmd.Flags().Set(flags.BuildrunNameFlag, "existing-br"); err != nil { + t.Fatal(err) + } + // buildref-name is set programmatically from the positional argument + if err := u.cmd.Flags().Set(flags.BuildrefNameFlag, "my-build"); err != nil { + t.Fatal(err) + } + + if err := u.checkBuildRunNameConflicts(); err != nil { + t.Errorf("expected no error, got: %s", err.Error()) + } + }) + + t.Run("no buildrun-name allows spec flags", func(t *testing.T) { + u := newUploadCommandForTest() + if err := u.cmd.Flags().Set(flags.ServiceAccountNameFlag, "builder"); err != nil { + t.Fatal(err) + } + + if err := u.checkBuildRunNameConflicts(); err != nil { + t.Errorf("expected no error, got: %s", err.Error()) + } + }) +} + +func TestUploadResolveBuildRunFetchesExisting(t *testing.T) { + existing := &buildv1beta1.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: metav1.NamespaceDefault, + Name: "existing-br", + }, + } + shpclientset := shpfake.NewSimpleClientset(existing) + kclientset := fake.NewSimpleClientset() + param := params.NewParamsForTest(kclientset, shpclientset, nil, genericclioptions.NewConfigFlags(true), metav1.NamespaceDefault, nil, nil) + + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() + + u := newUploadCommandForTest() + u.ioStreams = &ioStreams + u.buildRunName = "existing-br" + + br, err := u.resolveBuildRun(param) + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + if br.Name != "existing-br" { + t.Errorf("expected to fetch existing-br, got: %s", br.Name) + } + + // no BuildRun should have been created + list, err := shpclientset.ShipwrightV1beta1().BuildRuns(metav1.NamespaceDefault).List(u.cmd.Context(), metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(list.Items) != 1 { + t.Errorf("expected exactly 1 BuildRun (the pre-existing one), got: %d", len(list.Items)) + } +} diff --git a/pkg/shp/flags/buildrun.go b/pkg/shp/flags/buildrun.go index 2e801a24a..717741842 100644 --- a/pkg/shp/flags/buildrun.go +++ b/pkg/shp/flags/buildrun.go @@ -39,6 +39,7 @@ func BuildRunSpecFromFlags(flags *pflag.FlagSet) *buildv1beta1.BuildRunSpec { imageFlags(flags, "output", spec.Output) envFlags(flags, &spec.Env) paramValueFlag(flags, &spec.ParamValues) + stepResourcesFlag(flags, &spec.StepResources) imageLabelsFlags(flags, spec.Output.Labels) imageAnnotationsFlags(flags, spec.Output.Annotations) buildRunRetentionFlags(flags, spec.Retention) diff --git a/pkg/shp/flags/flags.go b/pkg/shp/flags/flags.go index 3e77b9c56..d3e813e0e 100644 --- a/pkg/shp/flags/flags.go +++ b/pkg/shp/flags/flags.go @@ -56,6 +56,10 @@ const ( OutputCredentialsSecretFlag = "output-credentials-secret" // #nosec G101 // ParamValueFlag command-line flag. ParamValueFlag = "param-value" + // StepResourcesFlag command-line flag. + StepResourcesFlag = "step-resources" + // BuildrunNameFlag command-line flag. + BuildrunNameFlag = "buildrun-name" // ServiceAccountNameFlag command-line flag. ServiceAccountNameFlag = "sa-name" // ServiceAccountGenerateFlag command-line flag. @@ -319,6 +323,16 @@ func paramValueFlag(flags *pflag.FlagSet, paramValue *[]buildv1beta1.ParamValue) ) } +// stepResourcesFlag registers flags for adding BuildRunSpec.StepResources. +func stepResourcesFlag(flags *pflag.FlagSet, stepResources *[]buildv1beta1.StepResourceOverride) { + flags.VarP( + NewStepResourcesArrayValue(stepResources), + StepResourcesFlag, + "", + "override compute resources for a build step, as =.= (repeatable)", + ) +} + // imageLabelsFlags registers flags for output image labels. func imageLabelsFlags(flags *pflag.FlagSet, labels map[string]string) { flags.VarP( diff --git a/pkg/shp/flags/step_resources_value.go b/pkg/shp/flags/step_resources_value.go new file mode 100644 index 000000000..e07ec009a --- /dev/null +++ b/pkg/shp/flags/step_resources_value.go @@ -0,0 +1,90 @@ +package flags + +import ( + "fmt" + "strings" + + buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// StepResourcesArrayValue implements the pflag.Value interface, storing per-step resource +// requirement overrides used on Shipwright's BuildRunSpec.StepResources. +type StepResourcesArrayValue struct { + stepResources *[]buildv1beta1.StepResourceOverride // pointer to the slice of overrides +} + +// String prints out the string representation of the slice of overrides. +func (s *StepResourcesArrayValue) String() string { + slice := []string{} + for _, o := range *s.stepResources { + slice = append(slice, o.Name) + } + csv, _ := writeAsCSV(slice) + return fmt.Sprintf("[%s]", csv) +} + +// Set receives an entry in the format "=.=", for example +// "build-and-push=limits.memory=2Gi". Repeated entries for the same step are merged. +func (s *StepResourcesArrayValue) Set(value string) error { + step, spec, err := splitKeyValue(value) + if err != nil { + return fmt.Errorf("step resources value %q is not in =.= format: %w", value, err) + } + + resKey, qtyStr, err := splitKeyValue(spec) + if err != nil { + return fmt.Errorf("step resources value %q is not in =.= format: %w", value, err) + } + + kind, resName, found := strings.Cut(resKey, ".") + if !found || resName == "" { + return fmt.Errorf("step resources value %q must specify the resource as .", value) + } + if kind != "limits" && kind != "requests" { + return fmt.Errorf("step resources value %q must use %q or %q, got %q", value, "limits", "requests", kind) + } + + qty, err := resource.ParseQuantity(qtyStr) + if err != nil { + return fmt.Errorf("step resources value %q has an invalid quantity %q: %w", value, qtyStr, err) + } + + override := s.getOrCreate(step) + if kind == "limits" { + if override.Resources.Limits == nil { + override.Resources.Limits = corev1.ResourceList{} + } + override.Resources.Limits[corev1.ResourceName(resName)] = qty + } else { + if override.Resources.Requests == nil { + override.Resources.Requests = corev1.ResourceList{} + } + override.Resources.Requests[corev1.ResourceName(resName)] = qty + } + return nil +} + +// getOrCreate returns a pointer to the override for the given step name, appending a new entry when +// one does not exist yet. +func (s *StepResourcesArrayValue) getOrCreate(step string) *buildv1beta1.StepResourceOverride { + for i := range *s.stepResources { + if (*s.stepResources)[i].Name == step { + return &(*s.stepResources)[i] + } + } + *s.stepResources = append(*s.stepResources, buildv1beta1.StepResourceOverride{Name: step}) + return &(*s.stepResources)[len(*s.stepResources)-1] +} + +// Type analogous to the pflag "stringArray" type, where each flag entry is translated to a single +// slice entry. +func (s *StepResourcesArrayValue) Type() string { + return "stringArray" +} + +// NewStepResourcesArrayValue instantiates a StepResourcesArrayValue sharing the slice pointer. +func NewStepResourcesArrayValue(stepResources *[]buildv1beta1.StepResourceOverride) *StepResourcesArrayValue { + return &StepResourcesArrayValue{stepResources: stepResources} +} diff --git a/pkg/shp/flags/step_resources_value_test.go b/pkg/shp/flags/step_resources_value_test.go new file mode 100644 index 000000000..bc4c8b2a1 --- /dev/null +++ b/pkg/shp/flags/step_resources_value_test.go @@ -0,0 +1,83 @@ +package flags + +import ( + "testing" + + "github.com/onsi/gomega" + buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +func TestNewStepResourcesArrayValue(t *testing.T) { + g := gomega.NewWithT(t) + + t.Run("single limit", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("build-and-push=limits.memory=2Gi")).To(gomega.BeNil()) + + g.Expect(spec.StepResources).To(gomega.HaveLen(1)) + g.Expect(spec.StepResources[0].Name).To(gomega.Equal("build-and-push")) + g.Expect(spec.StepResources[0].Resources.Limits).To(gomega.Equal(corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("2Gi"), + })) + }) + + t.Run("requests and limits merge on same step", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("build-and-push=limits.memory=2Gi")).To(gomega.BeNil()) + g.Expect(v.Set("build-and-push=requests.cpu=500m")).To(gomega.BeNil()) + + g.Expect(spec.StepResources).To(gomega.HaveLen(1)) + g.Expect(spec.StepResources[0].Resources.Limits).To(gomega.Equal(corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("2Gi"), + })) + g.Expect(spec.StepResources[0].Resources.Requests).To(gomega.Equal(corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + })) + }) + + t.Run("distinct steps append", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("step-a=limits.memory=1Gi")).To(gomega.BeNil()) + g.Expect(v.Set("step-b=limits.memory=2Gi")).To(gomega.BeNil()) + + g.Expect(spec.StepResources).To(gomega.HaveLen(2)) + g.Expect(spec.StepResources[0].Name).To(gomega.Equal("step-a")) + g.Expect(spec.StepResources[1].Name).To(gomega.Equal("step-b")) + }) + + t.Run("missing step separator", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("limits.memory=2Gi")).NotTo(gomega.BeNil()) + }) + + t.Run("missing resource value", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("build-and-push=limits.memory")).NotTo(gomega.BeNil()) + }) + + t.Run("unknown resource kind", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("build-and-push=foo.memory=2Gi")).NotTo(gomega.BeNil()) + }) + + t.Run("invalid quantity", func(_ *testing.T) { + spec := buildv1beta1.BuildRunSpec{} + v := NewStepResourcesArrayValue(&spec.StepResources) + + g.Expect(v.Set("build-and-push=limits.memory=notaquantity")).NotTo(gomega.BeNil()) + }) +} diff --git a/test/e2e/upload.bats b/test/e2e/upload.bats index 76a2cb118..bafb3cae5 100644 --- a/test/e2e/upload.bats +++ b/test/e2e/upload.bats @@ -74,6 +74,110 @@ function assert_shp_upload_follow_output() { assert_shp_upload_follow_output } +@test "shp build upload with step resources" { + build_name=$(random_name) + + output_image="registry.registry.svc.cluster.local:32222/shipwright-io/build-e2e" + source_url="https://github.com/shipwright-io/sample-go" + repo_dir="${BATS_TEST_TMPDIR}/sample-go" + + run shp build create ${build_name} \ + --source-git-url="${source_url}" \ + --source-context-dir="source-build" \ + --output-image="${output_image}" \ + --output-insecure=true + assert_success + + run git clone "${source_url}" "${repo_dir}" + assert_success + + # + # Test Cases + # + + # uploading with a per-step resource override, the created BuildRun must carry it on its spec + run shp build upload ${build_name} "${repo_dir}" \ + --step-resources=build-and-push=limits.memory=1Gi \ + --step-resources=build-and-push=requests.cpu=250m + assert_success + assert_shp_upload_output + + # the created BuildRun spec must contain the requested step resources + run kubectl get buildruns.shipwright.io \ + -o jsonpath='{.items[0].spec.stepResources[0].resources.limits.memory}' + assert_success + assert_output '1Gi' + + run kubectl get buildruns.shipwright.io \ + -o jsonpath='{.items[0].spec.stepResources[0].resources.requests.cpu}' + assert_success + assert_output '250m' + + # an invalid step-resources value must be rejected before anything is created + run shp build upload ${build_name} "${repo_dir}" \ + --step-resources=build-and-push=limits.memory=not-a-quantity + assert_failure +} + +@test "shp build upload into an existing BuildRun" { + build_name=$(random_name) + buildrun_name=$(random_name) + + output_image="registry.registry.svc.cluster.local:32222/shipwright-io/build-e2e" + source_url="https://github.com/shipwright-io/sample-go" + repo_dir="${BATS_TEST_TMPDIR}/sample-go" + + run shp build create ${build_name} \ + --source-git-url="${source_url}" \ + --source-context-dir="source-build" \ + --output-image="${output_image}" \ + --output-insecure=true + assert_success + + run git clone "${source_url}" "${repo_dir}" + assert_success + + # a BuildRun created out-of-band, carrying fields the upload command does not expose as flags + # (here, a local source so the streaming waiter container is present) + cat <<-EOF | kubectl create -f - + apiVersion: shipwright.io/v1beta1 + kind: BuildRun + metadata: + name: ${buildrun_name} + spec: + build: + name: ${build_name} + source: + type: Local + local: + name: local-copy + EOF + + # + # Test Cases + # + + # combining --buildrun-name with a creation-only flag must be rejected + run shp build upload ${build_name} "${repo_dir}" \ + --buildrun-name="${buildrun_name}" \ + --sa-name=builder + assert_failure + assert_output --partial 'buildrun-name' + + # streaming into the pre-created BuildRun, following logs to completion + run shp build upload --follow ${build_name} "${repo_dir}" \ + --buildrun-name="${buildrun_name}" + assert_success + assert_output --partial 'Streaming into existing BuildRun' + assert_output --partial 'to the Build POD' + assert_shp_upload_follow_output + + # no extra BuildRun should have been created; only the one we made by hand + run kubectl get buildruns.shipwright.io -o jsonpath='{.items[*].metadata.name}' + assert_success + assert_output "${buildrun_name}" +} + @test "shp build upload with bundle" { build_name=$(random_name) From aa15a2207354a4798180070663a84a38fc2690b4 Mon Sep 17 00:00:00 2001 From: Prateek Singh Rathore Date: Tue, 29 Sep 2026 13:20:33 +0530 Subject: [PATCH 2/2] fix: make --buildrun-name work with any namespace and a running pod Testing `shp build upload --buildrun-name` on a cluster showed four ways it failed. The flag check rejected global flags, so --namespace and --kubeconfig could not be used with --buildrun-name. The check now looks only at the upload command's own flags. The pod of an existing BuildRun is often already running when the upload starts. The upload only reacted to changes in the pod, and a waiting pod does not change, so it sat idle until the pod's upload waiter gave up and then failed with "container not found". It now also reacts to the first event it sees for the pod. A BuildRun of another Build made the command wait forever, because the upload finds the build pod by the Build name. A BuildRun without a Local source failed with an unclear error about a missing container, because its pod has nothing waiting for the upload. Both are now checked before the upload starts, and the error names the BuildRun. Unit tests cover each case and fail on the code before this commit. The e2e case now waits for the pod to be running before it uploads, passes --namespace, and checks the memory limit on the step container. Co-Authored-By: Claude Signed-off-by: Prateek Singh Rathore --- pkg/shp/cmd/build/upload.go | 20 +++- pkg/shp/cmd/build/upload_test.go | 154 ++++++++++++++++++++++++++++--- test/e2e/upload.bats | 21 ++++- 3 files changed, 178 insertions(+), 17 deletions(-) diff --git a/pkg/shp/cmd/build/upload.go b/pkg/shp/cmd/build/upload.go index 5927022c8..8c96eb175 100644 --- a/pkg/shp/cmd/build/upload.go +++ b/pkg/shp/cmd/build/upload.go @@ -168,7 +168,8 @@ func (u *UploadCommand) Validate() error { // checkBuildRunNameConflicts makes sure that, when an existing BuildRun is referenced with the // --buildrun-name flag, none of the flags that only apply to a BuildRun this command creates are -// also set, since those settings would be silently ignored. +// also set. Those flags would have no effect on an existing BuildRun, so they are rejected with an +// error. Global flags inherited from the root command, such as --namespace, are not checked. func (u *UploadCommand) checkBuildRunNameConflicts() error { if u.buildRunName == "" { return nil @@ -180,8 +181,8 @@ func (u *UploadCommand) checkBuildRunNameConflicts() error { "follow": true, } var conflicting []string - u.cmd.Flags().Visit(func(f *pflag.Flag) { - if !allowed[f.Name] { + u.cmd.LocalFlags().VisitAll(func(f *pflag.Flag) { + if f.Changed && !allowed[f.Name] { conflicting = append(conflicting, "--"+f.Name) } }) @@ -260,6 +261,14 @@ func (u *UploadCommand) resolveBuildRun(p *params.Params) (*buildv1beta1.BuildRu if err != nil { return nil, err } + // the build pod is found by the Build name, so a BuildRun of another Build would never be matched + if br.Spec.Build.Name == nil || *br.Spec.Build.Name != u.buildRefName { + return nil, fmt.Errorf("BuildRun '%s' does not reference Build '%s'", br.GetName(), u.buildRefName) + } + // without a Local source the build pod has no container waiting for the upload + if br.Spec.Source == nil || br.Spec.Source.Type != buildv1beta1.LocalType { + return nil, fmt.Errorf("BuildRun '%s' must have a source of type %q to receive an upload", br.GetName(), buildv1beta1.LocalType) + } fmt.Fprintf(u.ioStreams.Out, "Streaming into existing BuildRun '%s/%s'...\n", ns, br.GetName()) return br, nil } @@ -368,7 +377,10 @@ func (u *UploadCommand) Run(p *params.Params, ioStreams *genericclioptions.IOStr // Using streaming to upload local source code default: - // registering the routine that will react upon build pod state changes + // registering the routine that will react upon build pod state changes; it also handles the + // first event, since the pod of an existing BuildRun may already be running when the watch + // starts and would then only change again when the upload times out + u.pw.WithOnPodAddedFn(u.onPodModifiedEventStreaming) u.pw.WithOnPodModifiedFn(u.onPodModifiedEventStreaming) } diff --git a/pkg/shp/cmd/build/upload_test.go b/pkg/shp/cmd/build/upload_test.go index 97200d669..82da501fa 100644 --- a/pkg/shp/cmd/build/upload_test.go +++ b/pkg/shp/cmd/build/upload_test.go @@ -1,18 +1,24 @@ package build // nolint:revive import ( + "context" "strings" "testing" + "time" buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1" shpfake "github.com/shipwright-io/build/pkg/client/clientset/versioned/fake" "github.com/shipwright-io/cli/pkg/shp/flags" "github.com/shipwright-io/cli/pkg/shp/params" + "github.com/shipwright-io/cli/pkg/shp/reactor" "github.com/spf13/cobra" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/kubernetes/fake" + fakekubetesting "k8s.io/client-go/testing" ) // newUploadCommandForTest builds an UploadCommand wired with a fresh cobra command and the standard @@ -75,36 +81,160 @@ func TestUploadValidateBuildRunNameConflicts(t *testing.T) { } func TestUploadResolveBuildRunFetchesExisting(t *testing.T) { + localSource := &buildv1beta1.BuildRunSource{ + Type: buildv1beta1.LocalType, + Local: &buildv1beta1.Local{Name: "local-copy"}, + } + + tests := []struct { + name string + buildName string + source *buildv1beta1.BuildRunSource + wantErr string + }{ + {name: "matching Build and Local source", buildName: "my-build", source: localSource}, + {name: "another Build is rejected", buildName: "other-build", source: localSource, wantErr: "does not reference Build 'my-build'"}, + {name: "a source that is not Local is rejected", buildName: "my-build", source: &buildv1beta1.BuildRunSource{}, wantErr: "must have a source of type"}, + {name: "no source is rejected", buildName: "my-build", wantErr: "must have a source of type"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + existing := &buildv1beta1.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: metav1.NamespaceDefault, + Name: "existing-br", + }, + Spec: buildv1beta1.BuildRunSpec{ + Build: buildv1beta1.ReferencedBuild{Name: &tt.buildName}, + Source: tt.source, + }, + } + shpclientset := shpfake.NewSimpleClientset(existing) + kclientset := fake.NewSimpleClientset() + param := params.NewParamsForTest(kclientset, shpclientset, nil, genericclioptions.NewConfigFlags(true), metav1.NamespaceDefault, nil, nil) + + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() + + u := newUploadCommandForTest() + u.ioStreams = &ioStreams + u.buildRefName = "my-build" + u.buildRunName = "existing-br" + + br, err := u.resolveBuildRun(param) + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("expected an error containing %q, got: %v", tt.wantErr, err) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %s", err.Error()) + } + if br.Name != "existing-br" { + t.Errorf("expected to fetch existing-br, got: %s", br.Name) + } + } + + // no BuildRun should have been created + list, err := shpclientset.ShipwrightV1beta1().BuildRuns(metav1.NamespaceDefault).List(u.cmd.Context(), metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(list.Items) != 1 { + t.Errorf("expected exactly 1 BuildRun (the pre-existing one), got: %d", len(list.Items)) + } + }) + } +} + +func TestUploadValidateBuildRunNameAllowsGlobalFlags(t *testing.T) { + u := newUploadCommandForTest() + // the global flags, such as --namespace, are inherited from the root command + root := &cobra.Command{} + genericclioptions.NewConfigFlags(true).AddFlags(root.PersistentFlags()) + root.AddCommand(u.cmd) + + if err := u.cmd.ParseFlags([]string{"--namespace=other", "--buildrun-name=existing-br"}); err != nil { + t.Fatal(err) + } + + if err := u.checkBuildRunNameConflicts(); err != nil { + t.Errorf("expected no error, got: %s", err.Error()) + } +} + +func TestUploadValidateBuildRunNameRejectsSourceBundle(t *testing.T) { + u := newUploadCommandForTest() + u.sourceDir = t.TempDir() + u.sourceBundleImage = "registry.example.com/source-bundle:latest" + if err := u.cmd.Flags().Set(flags.BuildrunNameFlag, "existing-br"); err != nil { + t.Fatal(err) + } + + err := u.Validate() + if err == nil { + t.Fatal("expected an error, got nil") + } + if !strings.Contains(err.Error(), "source bundle") { + t.Errorf("error should mention the source bundle, got: %s", err.Error()) + } +} + +func TestUploadRunHandlesPodThatExistsBeforeTheWatch(t *testing.T) { + buildName := "my-build" existing := &buildv1beta1.BuildRun{ ObjectMeta: metav1.ObjectMeta{ Namespace: metav1.NamespaceDefault, Name: "existing-br", }, + Spec: buildv1beta1.BuildRunSpec{ + Build: buildv1beta1.ReferencedBuild{Name: &buildName}, + Source: &buildv1beta1.BuildRunSource{ + Type: buildv1beta1.LocalType, + Local: &buildv1beta1.Local{Name: "local-copy"}, + }, + }, } shpclientset := shpfake.NewSimpleClientset(existing) kclientset := fake.NewSimpleClientset() - param := params.NewParamsForTest(kclientset, shpclientset, nil, genericclioptions.NewConfigFlags(true), metav1.NamespaceDefault, nil, nil) + // the pod of an existing BuildRun can be past the point the upload waits for when the watch + // starts, so the only event the command receives for it is the first one, of type Added + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "existing-br-pod"}, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + watcher := watch.NewFakeWithChanSize(1, false) + watcher.Add(pod) + kclientset.PrependWatchReactor("pods", fakekubetesting.DefaultWatchReactor(watcher, nil)) + + param := params.NewParamsForTest(kclientset, shpclientset, nil, genericclioptions.NewConfigFlags(true), metav1.NamespaceDefault, nil, nil) ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() u := newUploadCommandForTest() + u.cmd.SetContext(context.Background()) u.ioStreams = &ioStreams + u.buildRefName = buildName u.buildRunName = "existing-br" - - br, err := u.resolveBuildRun(param) - if err != nil { - t.Fatalf("unexpected error: %s", err.Error()) - } - if br.Name != "existing-br" { - t.Errorf("expected to fetch existing-br, got: %s", br.Name) + if err := u.cmd.Flags().Set(flags.BuildrefNameFlag, buildName); err != nil { + t.Fatal(err) } - // no BuildRun should have been created - list, err := shpclientset.ShipwrightV1beta1().BuildRuns(metav1.NamespaceDefault).List(u.cmd.Context(), metav1.ListOptions{}) + pw, err := reactor.NewPodWatcher(context.Background(), time.Minute, kclientset, metav1.NamespaceDefault) if err != nil { t.Fatal(err) } - if len(list.Items) != 1 { - t.Errorf("expected exactly 1 BuildRun (the pre-existing one), got: %d", len(list.Items)) + u.pw = pw + + done := make(chan error, 1) + go func() { done <- u.Run(param, &ioStreams) }() + + select { + case err := <-done: + if err != nil { + t.Errorf("unexpected error: %s", err.Error()) + } + case <-time.After(10 * time.Second): + pw.Stop() + t.Fatal("upload did not react to the pod's first event and kept waiting") } } diff --git a/test/e2e/upload.bats b/test/e2e/upload.bats index bafb3cae5..08ac090a8 100644 --- a/test/e2e/upload.bats +++ b/test/e2e/upload.bats @@ -113,6 +113,12 @@ function assert_shp_upload_follow_output() { assert_success assert_output '250m' + # the build pod's step container must carry the requested memory limit + run kubectl get pods -l "build.shipwright.io/name=${build_name}" \ + -o jsonpath='{.items[0].spec.containers[?(@.name=="step-build-and-push")].resources.limits.memory}' + assert_success + assert_output '1Gi' + # an invalid step-resources value must be rejected before anything is created run shp build upload ${build_name} "${repo_dir}" \ --step-resources=build-and-push=limits.memory=not-a-quantity @@ -164,8 +170,21 @@ function assert_shp_upload_follow_output() { assert_failure assert_output --partial 'buildrun-name' - # streaming into the pre-created BuildRun, following logs to completion + # the pod of a BuildRun created earlier is usually already running when the upload starts; + # kubectl wait fails at once when no pod matches yet, so wait for the pod to exist first + for _ in $(seq 1 60); do + [ -n "$(kubectl get pods -l "buildrun.shipwright.io/name=${buildrun_name}" -o name)" ] && break + sleep 2 + done + run kubectl wait pods -l "buildrun.shipwright.io/name=${buildrun_name}" \ + --for=jsonpath='{.status.phase}'=Running --timeout=180s + assert_success + + # streaming into the pre-created BuildRun, following logs to completion; a global flag such as + # --namespace is accepted alongside --buildrun-name + ns="${TEST_NAMESPACE:-default}" run shp build upload --follow ${build_name} "${repo_dir}" \ + --namespace="${ns}" \ --buildrun-name="${buildrun_name}" assert_success assert_output --partial 'Streaming into existing BuildRun'