From 6d345206565f6179725031661d6d66ca86cba018 Mon Sep 17 00:00:00 2001 From: Caroline Scherf Date: Tue, 29 Aug 2023 17:00:45 +0100 Subject: [PATCH 1/2] Adds flag success-build-history-limit and failed-build-history-limit without defaults --- docs/kp_image_create.md | 2 + docs/kp_image_patch.md | 2 + docs/kp_image_save.md | 2 + pkg/commands/image/create.go | 2 + pkg/commands/image/create_test.go | 206 +++++++++++++++++++- pkg/commands/image/patch.go | 2 + pkg/commands/image/save.go | 2 + pkg/commands/image/testdata/test-image.json | 4 +- pkg/commands/image/testdata/test-image.yaml | 4 +- pkg/image/factory.go | 32 +++ pkg/image/factory_test.go | 10 + 11 files changed, 262 insertions(+), 6 deletions(-) diff --git a/docs/kp_image_create.md b/docs/kp_image_create.md index ee5c8b6..538fbe0 100644 --- a/docs/kp_image_create.md +++ b/docs/kp_image_create.md @@ -62,6 +62,7 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables + --failed-build-history-limit string set the failedBuildHistoryLimit --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for create @@ -77,6 +78,7 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho --service-account string service account name to use (default "default") -s, --service-binding stringArray build time service bindings --sub-path string build code at the sub path located within the source code directory + --success-build-history-limit string set the successBuildHistoryLimit -t, --tag string registry location where the OCI image will be created -w, --wait wait for image create to be reconciled and tail resulting build logs ``` diff --git a/docs/kp_image_patch.md b/docs/kp_image_patch.md index 06dc8c5..5b72eee 100644 --- a/docs/kp_image_patch.md +++ b/docs/kp_image_patch.md @@ -65,6 +65,7 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables to add/replace + --failed-build-history-limit string set the failedBuildHistoryLimit --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for patch @@ -79,6 +80,7 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos --service-account string service account name to use -s, --service-binding stringArray build time service bindings to add/replace --sub-path string build code at the sub path located within the source code directory + --success-build-history-limit string set the successBuildHistoryLimit -w, --wait wait for image resource patch to be reconciled and tail resulting build logs ``` diff --git a/docs/kp_image_save.md b/docs/kp_image_save.md index 0c07dc6..0dae6cb 100644 --- a/docs/kp_image_save.md +++ b/docs/kp_image_save.md @@ -68,6 +68,7 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables + --failed-build-history-limit string set the failedBuildHistoryLimit --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for save @@ -83,6 +84,7 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host --service-account string service account name to use -s, --service-binding stringArray build time service bindings to add/replace --sub-path string build code at the sub path located within the source code directory + --success-build-history-limit string set the successBuildHistoryLimit -t, --tag string registry location where the image will be created -w, --wait wait for image create to be reconciled and tail resulting build logs ``` diff --git a/pkg/commands/image/create.go b/pkg/commands/image/create.go index 002d808..f5554b1 100644 --- a/pkg/commands/image/create.go +++ b/pkg/commands/image/create.go @@ -108,6 +108,8 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho cmd.Flags().StringArrayVarP(&factory.Env, "env", "e", []string{}, "build time environment variables") cmd.Flags().StringArrayVarP(&factory.ServiceBinding, "service-binding", "s", []string{}, "build time service bindings") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity (default \"2G\")") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") cmd.Flags().StringVar(&factory.ServiceAccount, "service-account", "default", "service account name to use") cmd.Flags().BoolP("wait", "w", false, "wait for image create to be reconciled and tail resulting build logs") commands.SetImgUploadDryRunOutputFlags(cmd) diff --git a/pkg/commands/image/create_test.go b/pkg/commands/image/create_test.go index b809bbd..d0e96f4 100644 --- a/pkg/commands/image/create_test.go +++ b/pkg/commands/image/create_test.go @@ -6,8 +6,11 @@ package image_test import ( "encoding/json" "os" + "strconv" "testing" + registryfakes "github.com/vmware-tanzu/kpack-cli/pkg/registry/fakes" + "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1" "github.com/pivotal/kpack/pkg/client/clientset/versioned/fake" @@ -24,7 +27,6 @@ import ( imgcmds "github.com/vmware-tanzu/kpack-cli/pkg/commands/image" "github.com/vmware-tanzu/kpack-cli/pkg/k8s" "github.com/vmware-tanzu/kpack-cli/pkg/registry" - registryfakes "github.com/vmware-tanzu/kpack-cli/pkg/registry/fakes" "github.com/vmware-tanzu/kpack-cli/pkg/testhelpers" ) @@ -46,7 +48,6 @@ func testCreateCommand(imageCommand func(clientSetProvider k8s.ClientSetProvider const defaultNamespace = "some-default-namespace" registryUtilProvider := registryfakes.UtilProvider{} - fakeImageWaiter := &cmdFakes.FakeImageWaiter{} cmdFunc := func(clientSet *fake.Clientset) *cobra.Command { @@ -309,6 +310,100 @@ Image Resource "some-image" created }) }) + when("build history limits are provided", func() { + when("the image config is valid", func() { + it("creates the image with build history limits", func() { + buildHistoryLimit := int64(5) + defaultBuildHistoryLimit := &buildHistoryLimit + + expectedImage := &v1alpha2.Image{ + TypeMeta: metav1.TypeMeta{ + Kind: "Image", + APIVersion: "kpack.io/v1alpha2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "some-image", + Namespace: defaultNamespace, + Annotations: map[string]string{}, + }, + Spec: v1alpha2.ImageSpec{ + Tag: "some-registry.io/some-repo", + Builder: corev1.ObjectReference{ + Kind: v1alpha2.ClusterBuilderKind, + Name: "default", + }, + ServiceAccountName: "default", + Source: corev1alpha1.SourceConfig{ + Git: &corev1alpha1.Git{ + URL: "some-git-url", + Revision: "some-git-rev", + }, + SubPath: "some-sub-path", + }, + SuccessBuildHistoryLimit: defaultBuildHistoryLimit, + FailedBuildHistoryLimit: defaultBuildHistoryLimit, + Build: &v1alpha2.ImageBuild{ + Env: []corev1.EnvVar{ + { + Name: "some-key", + Value: "some-val", + }, + }, + Services: v1alpha2.Services{ + { + APIVersion: "v1", + Kind: "SomeResource", + Name: "some-binding", + }, + }, + }, + }, + } + + require.NoError(t, setLastAppliedAnnotation(expectedImage)) + testhelpers.CommandTest{ + Args: []string{ + "some-image", + "--tag", "some-registry.io/some-repo", + "--git", "some-git-url", + "--git-revision", "some-git-rev", + "--sub-path", "some-sub-path", + "--env", "some-key=some-val", + "--success-build-history-limit", "5", + "--failed-build-history-limit", "5", + "--service-binding", "SomeResource:v1:some-binding", + }, + ExpectedOutput: `Creating Image Resource... +Image Resource "some-image" created +`, + ExpectCreates: []runtime.Object{ + expectedImage, + }, + }.TestKpack(t, cmdFunc) + + assert.Len(t, fakeImageWaiter.Calls, 0) + }) + }) + + when("the image config is invalid", func() { + it("returns an error", func() { + testhelpers.CommandTest{ + Args: []string{ + "some-image", + "--tag", "some-registry.io/some-repo", + "--blob", "some-blob", + "--git", "some-git-url", + }, + ExpectErr: true, + ExpectedOutput: "Creating Image Resource...\n", + ExpectedErrorOutput: "Error: image source must be one of git, blob, or local-path\n", + }.TestKpack(t, cmdFunc) + + assert.Len(t, fakeImageWaiter.Calls, 0) + }) + }) + }) + when("the image uses local source code", func() { it("uploads the source image and creates the image config", func() { expectedImage := &v1alpha2.Image{ @@ -443,6 +538,9 @@ Image Resource "some-image" created when("the image uses a non-default builder", func() { it("uploads the source image and creates the image config", func() { + buildHistoryLimit := int64(10) + defaultBuildHistoryLimit := &buildHistoryLimit + expectedImage := &v1alpha2.Image{ TypeMeta: metav1.TypeMeta{ Kind: "Image", @@ -466,7 +564,9 @@ Image Resource "some-image" created URL: "some-blob", }, }, - Build: &v1alpha2.ImageBuild{}, + Build: &v1alpha2.ImageBuild{}, + SuccessBuildHistoryLimit: defaultBuildHistoryLimit, + FailedBuildHistoryLimit: defaultBuildHistoryLimit, }, } require.NoError(t, setLastAppliedAnnotation(expectedImage)) @@ -477,6 +577,8 @@ Image Resource "some-image" created "--tag", "some-registry.io/some-repo", "--blob", "some-blob", "--builder", "some-builder", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), }, ExpectedOutput: `Creating Image Resource... Image Resource "some-image" created @@ -492,6 +594,9 @@ Image Resource "some-image" created when("the image uses a non-default cluster builder", func() { it("uploads the source image and creates the image config", func() { + buildHistoryLimit := int64(5) + defaultBuildHistoryLimit := &buildHistoryLimit + expectedImage := &v1alpha2.Image{ TypeMeta: metav1.TypeMeta{ Kind: "Image", @@ -514,7 +619,9 @@ Image Resource "some-image" created URL: "some-blob", }, }, - Build: &v1alpha2.ImageBuild{}, + Build: &v1alpha2.ImageBuild{}, + SuccessBuildHistoryLimit: defaultBuildHistoryLimit, + FailedBuildHistoryLimit: defaultBuildHistoryLimit, }, } require.NoError(t, setLastAppliedAnnotation(expectedImage)) @@ -525,6 +632,8 @@ Image Resource "some-image" created "--tag", "some-registry.io/some-repo", "--blob", "some-blob", "--cluster-builder", "some-builder", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), }, ExpectedOutput: `Creating Image Resource... Image Resource "some-image" created @@ -608,6 +717,9 @@ Image Resource "some-image" created }) when("the image config is valid", func() { + buildHistoryLimit := int64(10) + defaultBuildHistoryLimit := &buildHistoryLimit + expectedImage := &v1alpha2.Image{ TypeMeta: metav1.TypeMeta{ Kind: "Image", @@ -632,6 +744,8 @@ Image Resource "some-image" created }, SubPath: "some-sub-path", }, + FailedBuildHistoryLimit: defaultBuildHistoryLimit, + SuccessBuildHistoryLimit: defaultBuildHistoryLimit, Build: &v1alpha2.ImageBuild{ Env: []corev1.EnvVar{ { @@ -666,6 +780,8 @@ Image Resource "some-image" created "--sub-path", "some-sub-path", "--env", "some-key=some-val", "--service-binding", "SomeResource:v1:some-binding", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), "--output", "yaml", "--wait", }, @@ -680,6 +796,8 @@ Image Resource "some-image" created }) it("can output in json format and does not wait", func() { + buildHistoryLimit := int64(10) + require.NoError(t, setLastAppliedAnnotation(expectedImage)) resourceJSON, err := getTestFile("./testdata/test-image.json") if err != nil { @@ -695,6 +813,8 @@ Image Resource "some-image" created "--sub-path", "some-sub-path", "--env", "some-key=some-val", "--service-binding", "SomeResource:v1:some-binding", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), "--output", "json", "--wait", }, @@ -729,6 +849,7 @@ Image Resource "some-image" created when("the image config is valid", func() { it("does not creates an image and prints result message with dry run indicated", func() { + buildHistoryLimit := int64(10) testhelpers.CommandTest{ Args: []string{ "some-image", @@ -738,6 +859,8 @@ Image Resource "some-image" created "--sub-path", "some-sub-path", "--service-binding", "SomeResource:v1:some-binding", "--env", "some-key=some-val", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), "--dry-run", "--wait", }, @@ -754,6 +877,7 @@ Image Resource "some-image" created (dry run) if err != nil { t.Fatalf("unable to convert test file to string: %v", err) } + buildHistoryLimit := int64(10) testhelpers.CommandTest{ Args: []string{ @@ -764,6 +888,8 @@ Image Resource "some-image" created (dry run) "--sub-path", "some-sub-path", "--env", "some-key=some-val", "--service-binding", "SomeResource:v1:some-binding", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), "--output", "yaml", "--dry-run", "--wait", @@ -824,6 +950,7 @@ Image Resource "some-image" created (dry run with image upload) if err != nil { t.Fatalf("unable to convert test file to string: %v", err) } + buildHistoryLimit := int64(10) testhelpers.CommandTest{ Args: []string{ @@ -834,6 +961,8 @@ Image Resource "some-image" created (dry run with image upload) "--sub-path", "some-sub-path", "--env", "some-key=some-val", "--service-binding", "SomeResource:v1:some-binding", + "--success-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), + "--failed-build-history-limit", strconv.FormatInt(buildHistoryLimit, 10), "--output", "yaml", "--dry-run-with-image-upload", "--wait", @@ -847,6 +976,75 @@ Image Resource "some-image" created (dry run with image upload) }) }) }) + + when("cache size is not provided", func() { + it("does not set an empty field on the image", func() { + // note: this is to allow for defaults to be set by kpack webhook + + expectedImage := &v1alpha2.Image{ + TypeMeta: metav1.TypeMeta{ + Kind: "Image", + APIVersion: "kpack.io/v1alpha2", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "some-image", + Namespace: defaultNamespace, + Annotations: map[string]string{}, + }, + Spec: v1alpha2.ImageSpec{ + Tag: "some-registry.io/some-repo", + Builder: corev1.ObjectReference{ + Kind: v1alpha2.ClusterBuilderKind, + Name: "default", + }, + ServiceAccountName: "default", + Source: corev1alpha1.SourceConfig{ + Git: &corev1alpha1.Git{ + URL: "some-git-url", + Revision: "some-git-rev", + }, + SubPath: "some-sub-path", + }, + Build: &v1alpha2.ImageBuild{ + Env: []corev1.EnvVar{ + { + Name: "some-key", + Value: "some-val", + }, + }, + Services: v1alpha2.Services{ + { + APIVersion: "v1", + Kind: "SomeResource", + Name: "some-binding", + }, + }, + }, + }, + } + + require.NoError(t, setLastAppliedAnnotation(expectedImage)) + testhelpers.CommandTest{ + Args: []string{ + "some-image", + "--tag", "some-registry.io/some-repo", + "--git", "some-git-url", + "--git-revision", "some-git-rev", + "--sub-path", "some-sub-path", + "--env", "some-key=some-val", + "--service-binding", "SomeResource:v1:some-binding", + }, + ExpectedOutput: `Creating Image Resource... +Image Resource "some-image" created +`, + ExpectCreates: []runtime.Object{ + expectedImage, + }, + }.TestKpack(t, cmdFunc) + + assert.Len(t, fakeImageWaiter.Calls, 0) + }) + }) } } diff --git a/pkg/commands/image/patch.go b/pkg/commands/image/patch.go index 9785ee4..02c79a5 100644 --- a/pkg/commands/image/patch.go +++ b/pkg/commands/image/patch.go @@ -118,6 +118,8 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos cmd.Flags().StringArrayVarP(&factory.ServiceBinding, "service-binding", "s", []string{}, "build time service bindings to add/replace") cmd.Flags().StringArrayVarP(&factory.DeleteServiceBinding, "delete-service-binding", "", []string{}, "build time service bindings to remove") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") cmd.Flags().StringVar(&factory.ServiceAccount, "service-account", "", "service account name to use") cmd.Flags().BoolP("wait", "w", false, "wait for image resource patch to be reconciled and tail resulting build logs") commands.SetImgUploadDryRunOutputFlags(cmd) diff --git a/pkg/commands/image/save.go b/pkg/commands/image/save.go index 812510f..d8ff327 100644 --- a/pkg/commands/image/save.go +++ b/pkg/commands/image/save.go @@ -129,6 +129,8 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host cmd.Flags().StringVar(&factory.LocalPathDestinationImage, "local-path-destination-image", "", "registry location of where the local source code will be uploaded to (default \"-source\")") cmd.Flags().StringVar(&subPath, "sub-path", "", "build code at the sub path located within the source code directory") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity (default \"2G\")") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") cmd.Flags().StringVarP(&factory.Builder, "builder", "b", "", "builder name") cmd.Flags().StringVarP(&factory.ClusterBuilder, "cluster-builder", "c", "", "cluster builder name") cmd.Flags().StringArrayVarP(&factory.Env, "env", "e", []string{}, "build time environment variables") diff --git a/pkg/commands/image/testdata/test-image.json b/pkg/commands/image/testdata/test-image.json index 059dbc6..d51b3cf 100644 --- a/pkg/commands/image/testdata/test-image.json +++ b/pkg/commands/image/testdata/test-image.json @@ -6,7 +6,7 @@ "namespace": "some-default-namespace", "creationTimestamp": null, "annotations": { - "kubectl.kubernetes.io/last-applied-configuration": "{\"kind\":\"Image\",\"apiVersion\":\"kpack.io/v1alpha2\",\"metadata\":{\"name\":\"some-image\",\"namespace\":\"some-default-namespace\",\"creationTimestamp\":null},\"spec\":{\"tag\":\"some-registry.io/some-repo\",\"builder\":{\"kind\":\"ClusterBuilder\",\"name\":\"default\"},\"serviceAccountName\":\"default\",\"source\":{\"git\":{\"url\":\"some-git-url\",\"revision\":\"some-git-rev\"},\"subPath\":\"some-sub-path\"},\"build\":{\"services\":[{\"kind\":\"SomeResource\",\"name\":\"some-binding\",\"apiVersion\":\"v1\"}],\"env\":[{\"name\":\"some-key\",\"value\":\"some-val\"}],\"resources\":{}}},\"status\":{}}" + "kubectl.kubernetes.io/last-applied-configuration": "{\"kind\":\"Image\",\"apiVersion\":\"kpack.io/v1alpha2\",\"metadata\":{\"name\":\"some-image\",\"namespace\":\"some-default-namespace\",\"creationTimestamp\":null},\"spec\":{\"tag\":\"some-registry.io/some-repo\",\"builder\":{\"kind\":\"ClusterBuilder\",\"name\":\"default\"},\"serviceAccountName\":\"default\",\"source\":{\"git\":{\"url\":\"some-git-url\",\"revision\":\"some-git-rev\"},\"subPath\":\"some-sub-path\"},\"failedBuildHistoryLimit\":10,\"successBuildHistoryLimit\":10,\"build\":{\"services\":[{\"kind\":\"SomeResource\",\"name\":\"some-binding\",\"apiVersion\":\"v1\"}],\"env\":[{\"name\":\"some-key\",\"value\":\"some-val\"}],\"resources\":{}}},\"status\":{}}" } }, "spec": { @@ -23,6 +23,8 @@ }, "subPath": "some-sub-path" }, + "failedBuildHistoryLimit": 10, + "successBuildHistoryLimit": 10, "build": { "services": [ { diff --git a/pkg/commands/image/testdata/test-image.yaml b/pkg/commands/image/testdata/test-image.yaml index c017948..56e9cda 100644 --- a/pkg/commands/image/testdata/test-image.yaml +++ b/pkg/commands/image/testdata/test-image.yaml @@ -2,7 +2,7 @@ apiVersion: kpack.io/v1alpha2 kind: Image metadata: annotations: - kubectl.kubernetes.io/last-applied-configuration: '{"kind":"Image","apiVersion":"kpack.io/v1alpha2","metadata":{"name":"some-image","namespace":"some-default-namespace","creationTimestamp":null},"spec":{"tag":"some-registry.io/some-repo","builder":{"kind":"ClusterBuilder","name":"default"},"serviceAccountName":"default","source":{"git":{"url":"some-git-url","revision":"some-git-rev"},"subPath":"some-sub-path"},"build":{"services":[{"kind":"SomeResource","name":"some-binding","apiVersion":"v1"}],"env":[{"name":"some-key","value":"some-val"}],"resources":{}}},"status":{}}' + kubectl.kubernetes.io/last-applied-configuration: '{"kind":"Image","apiVersion":"kpack.io/v1alpha2","metadata":{"name":"some-image","namespace":"some-default-namespace","creationTimestamp":null},"spec":{"tag":"some-registry.io/some-repo","builder":{"kind":"ClusterBuilder","name":"default"},"serviceAccountName":"default","source":{"git":{"url":"some-git-url","revision":"some-git-rev"},"subPath":"some-sub-path"},"failedBuildHistoryLimit":10,"successBuildHistoryLimit":10,"build":{"services":[{"kind":"SomeResource","name":"some-binding","apiVersion":"v1"}],"env":[{"name":"some-key","value":"some-val"}],"resources":{}}},"status":{}}' creationTimestamp: null name: some-image namespace: some-default-namespace @@ -19,11 +19,13 @@ spec: builder: kind: ClusterBuilder name: default + failedBuildHistoryLimit: 10 serviceAccountName: default source: git: revision: some-git-rev url: some-git-url subPath: some-sub-path + successBuildHistoryLimit: 10 tag: some-registry.io/some-repo status: {} diff --git a/pkg/image/factory.go b/pkg/image/factory.go index acd217c..44c6b22 100644 --- a/pkg/image/factory.go +++ b/pkg/image/factory.go @@ -6,6 +6,7 @@ package image import ( "io" "sort" + "strconv" "strings" "github.com/google/go-containerregistry/pkg/authn" @@ -52,6 +53,8 @@ type Factory struct { Env []string ServiceBinding []string CacheSize string + SuccessBuildHistoryLimit string + FailedBuildHistoryLimit string DeleteEnv []string DeleteAdditionalTags []string DeleteServiceBinding []string @@ -121,6 +124,22 @@ func (f *Factory) MakeImage(name, namespace, tag string) (*v1alpha2.Image, error } } + if f.SuccessBuildHistoryLimit != "" { + successBuildHistoryLimit, err := strconv.ParseInt(f.SuccessBuildHistoryLimit, 10, 64) + if successBuildHistoryLimit < 1 || err != nil { + return nil, errors.New("must provide a valid success-build-history-limit > 0") + } + image.Spec.SuccessBuildHistoryLimit = &successBuildHistoryLimit + } + + if f.FailedBuildHistoryLimit != "" { + failedBuildHistoryLimit, err := strconv.ParseInt(f.FailedBuildHistoryLimit, 10, 64) + if failedBuildHistoryLimit < 1 || err != nil { + return nil, errors.New("must provide a valid failed-build-history-limit > 0") + } + image.Spec.FailedBuildHistoryLimit = &failedBuildHistoryLimit + } + return image, nil } @@ -223,6 +242,19 @@ func (f *Factory) getCacheSize() (*resource.Quantity, error) { return &c, nil } +func (f *Factory) makeBuildHistoryLimit(buildHistoryLimit string) (*int64, error) { + if buildHistoryLimit == "" { + return nil, nil + } + + value, err := strconv.ParseInt(buildHistoryLimit, 10, 64) + if err != nil || value < 1 { + return nil, errors.New("must provide a valid build history limit >1") + } + + return &value, nil +} + func (f *Factory) makeSource(tag string) (corev1alpha1.SourceConfig, error) { subPath := "" if f.SubPath != nil { diff --git a/pkg/image/factory_test.go b/pkg/image/factory_test.go index 223c7f0..72ef6ac 100644 --- a/pkg/image/factory_test.go +++ b/pkg/image/factory_test.go @@ -80,6 +80,16 @@ func testImageFactory(t *testing.T, when spec.G, it spec.S) { }) }) + when("an invalid build history limit is provided", func() { + it("returns an error message", func() { + factory.SuccessBuildHistoryLimit = "-1" + factory.FailedBuildHistoryLimit = "10" + factory.Blob = "some-blob" + _, err := factory.MakeImage("test-name", "test-namespace", "test-registry.io/test-image") + require.EqualError(t, err, "must provide a valid success-build-history-limit > 0") + }) + }) + when("cache size", func() { factory.Blob = "some-blob" From 7c4fe51edeed3699724b8c006b85932d4247c3e1 Mon Sep 17 00:00:00 2001 From: Caroline Scherf Date: Thu, 31 Aug 2023 10:41:40 +0100 Subject: [PATCH 2/2] Updates `success-build-history-limit and failed-build-history-limit` description, adds both commands to `patch` Signed-off-by: Caroline Scherf --- docs/kp_image_create.md | 4 ++-- docs/kp_image_patch.md | 4 ++-- docs/kp_image_save.md | 4 ++-- pkg/commands/image/create.go | 4 ++-- pkg/commands/image/patch.go | 4 ++-- pkg/commands/image/patch_test.go | 20 ++++++++++++++++++++ pkg/commands/image/save.go | 4 ++-- pkg/image/factory.go | 21 +++++++-------------- pkg/image/factory_test.go | 2 +- pkg/image/update_factory.go | 22 ++++++++++++++++++++++ 10 files changed, 62 insertions(+), 27 deletions(-) diff --git a/docs/kp_image_create.md b/docs/kp_image_create.md index 538fbe0..2e78a2e 100644 --- a/docs/kp_image_create.md +++ b/docs/kp_image_create.md @@ -62,7 +62,7 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables - --failed-build-history-limit string set the failedBuildHistoryLimit + --failed-build-history-limit string number of failed builds to keep, leave empty to use cluster default --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for create @@ -78,7 +78,7 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho --service-account string service account name to use (default "default") -s, --service-binding stringArray build time service bindings --sub-path string build code at the sub path located within the source code directory - --success-build-history-limit string set the successBuildHistoryLimit + --success-build-history-limit string number of successful builds to keep, leave empty to use cluster default -t, --tag string registry location where the OCI image will be created -w, --wait wait for image create to be reconciled and tail resulting build logs ``` diff --git a/docs/kp_image_patch.md b/docs/kp_image_patch.md index 5b72eee..3dc863f 100644 --- a/docs/kp_image_patch.md +++ b/docs/kp_image_patch.md @@ -65,7 +65,7 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables to add/replace - --failed-build-history-limit string set the failedBuildHistoryLimit + --failed-build-history-limit string number of failed builds to keep, leave empty to use cluster default --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for patch @@ -80,7 +80,7 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos --service-account string service account name to use -s, --service-binding stringArray build time service bindings to add/replace --sub-path string build code at the sub path located within the source code directory - --success-build-history-limit string set the successBuildHistoryLimit + --success-build-history-limit string number of successful builds to keep, leave empty to use cluster default -w, --wait wait for image resource patch to be reconciled and tail resulting build logs ``` diff --git a/docs/kp_image_save.md b/docs/kp_image_save.md index 0dae6cb..cd25c24 100644 --- a/docs/kp_image_save.md +++ b/docs/kp_image_save.md @@ -68,7 +68,7 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host resource with generated container image references. A "kubectl apply -f" of the resource from --output without image uploads will result in a reconcile failure. -e, --env stringArray build time environment variables - --failed-build-history-limit string set the failedBuildHistoryLimit + --failed-build-history-limit string number of failed builds to keep, leave empty to use cluster default --git string git repository url --git-revision string git revision such as commit, tag, or branch (default "main") -h, --help help for save @@ -84,7 +84,7 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host --service-account string service account name to use -s, --service-binding stringArray build time service bindings to add/replace --sub-path string build code at the sub path located within the source code directory - --success-build-history-limit string set the successBuildHistoryLimit + --success-build-history-limit string number of successful builds to keep, leave empty to use cluster default -t, --tag string registry location where the image will be created -w, --wait wait for image create to be reconciled and tail resulting build logs ``` diff --git a/pkg/commands/image/create.go b/pkg/commands/image/create.go index f5554b1..e2d8005 100644 --- a/pkg/commands/image/create.go +++ b/pkg/commands/image/create.go @@ -108,8 +108,8 @@ kp image create my-image --tag my-registry.com/my-repo --blob https://my-blob-ho cmd.Flags().StringArrayVarP(&factory.Env, "env", "e", []string{}, "build time environment variables") cmd.Flags().StringArrayVarP(&factory.ServiceBinding, "service-binding", "s", []string{}, "build time service bindings") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity (default \"2G\")") - cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") - cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "number of successful builds to keep, leave empty to use cluster default") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "number of failed builds to keep, leave empty to use cluster default") cmd.Flags().StringVar(&factory.ServiceAccount, "service-account", "default", "service account name to use") cmd.Flags().BoolP("wait", "w", false, "wait for image create to be reconciled and tail resulting build logs") commands.SetImgUploadDryRunOutputFlags(cmd) diff --git a/pkg/commands/image/patch.go b/pkg/commands/image/patch.go index 02c79a5..df3dbf2 100644 --- a/pkg/commands/image/patch.go +++ b/pkg/commands/image/patch.go @@ -118,8 +118,8 @@ kp image patch my-image --tag my-registry.com/my-repo --blob https://my-blob-hos cmd.Flags().StringArrayVarP(&factory.ServiceBinding, "service-binding", "s", []string{}, "build time service bindings to add/replace") cmd.Flags().StringArrayVarP(&factory.DeleteServiceBinding, "delete-service-binding", "", []string{}, "build time service bindings to remove") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity") - cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") - cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "number of successful builds to keep, leave empty to use cluster default") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "number of failed builds to keep, leave empty to use cluster default") cmd.Flags().StringVar(&factory.ServiceAccount, "service-account", "", "service account name to use") cmd.Flags().BoolP("wait", "w", false, "wait for image resource patch to be reconciled and tail resulting build logs") commands.SetImgUploadDryRunOutputFlags(cmd) diff --git a/pkg/commands/image/patch_test.go b/pkg/commands/image/patch_test.go index ca6f5bf..da90cdd 100644 --- a/pkg/commands/image/patch_test.go +++ b/pkg/commands/image/patch_test.go @@ -121,6 +121,26 @@ Image Resource "some-image" patched assert.Len(t, fakeImageWaiter.Calls, 0) }) + it("can patch the SuccessBuildHistoryLimit and FailedBuildHistoryLimit", func() { + testhelpers.CommandTest{ + Objects: []runtime.Object{ + existingImage, + }, + Args: []string{ + "some-image", + "--success-build-history-limit", "5", + "--failed-build-history-limit", "2", + }, + ExpectedOutput: `Patching Image Resource... +Image Resource "some-image" patched +`, + ExpectPatches: []string{ + `{"spec":{"failedBuildHistoryLimit":2,"successBuildHistoryLimit":5}}`, + }, + }.TestKpack(t, cmdFunc) + assert.Len(t, fakeImageWaiter.Calls, 0) + }) + when("patching source", func() { when("patching the sub path", func() { it("can patch it with an empty string", func() { diff --git a/pkg/commands/image/save.go b/pkg/commands/image/save.go index d8ff327..e867437 100644 --- a/pkg/commands/image/save.go +++ b/pkg/commands/image/save.go @@ -129,8 +129,8 @@ kp image save my-image --tag my-registry.com/my-repo --blob https://my-blob-host cmd.Flags().StringVar(&factory.LocalPathDestinationImage, "local-path-destination-image", "", "registry location of where the local source code will be uploaded to (default \"-source\")") cmd.Flags().StringVar(&subPath, "sub-path", "", "build code at the sub path located within the source code directory") cmd.Flags().StringVar(&factory.CacheSize, "cache-size", "", "cache size as a kubernetes quantity (default \"2G\")") - cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "set the successBuildHistoryLimit") - cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "set the failedBuildHistoryLimit") + cmd.Flags().StringVar(&factory.SuccessBuildHistoryLimit, "success-build-history-limit", "", "number of successful builds to keep, leave empty to use cluster default") + cmd.Flags().StringVar(&factory.FailedBuildHistoryLimit, "failed-build-history-limit", "", "number of failed builds to keep, leave empty to use cluster default") cmd.Flags().StringVarP(&factory.Builder, "builder", "b", "", "builder name") cmd.Flags().StringVarP(&factory.ClusterBuilder, "cluster-builder", "c", "", "cluster builder name") cmd.Flags().StringArrayVarP(&factory.Env, "env", "e", []string{}, "build time environment variables") diff --git a/pkg/image/factory.go b/pkg/image/factory.go index 44c6b22..ef823c0 100644 --- a/pkg/image/factory.go +++ b/pkg/image/factory.go @@ -124,20 +124,13 @@ func (f *Factory) MakeImage(name, namespace, tag string) (*v1alpha2.Image, error } } - if f.SuccessBuildHistoryLimit != "" { - successBuildHistoryLimit, err := strconv.ParseInt(f.SuccessBuildHistoryLimit, 10, 64) - if successBuildHistoryLimit < 1 || err != nil { - return nil, errors.New("must provide a valid success-build-history-limit > 0") - } - image.Spec.SuccessBuildHistoryLimit = &successBuildHistoryLimit + image.Spec.SuccessBuildHistoryLimit, err = f.makeBuildHistoryLimit(f.SuccessBuildHistoryLimit) + if err != nil { + return nil, err } - - if f.FailedBuildHistoryLimit != "" { - failedBuildHistoryLimit, err := strconv.ParseInt(f.FailedBuildHistoryLimit, 10, 64) - if failedBuildHistoryLimit < 1 || err != nil { - return nil, errors.New("must provide a valid failed-build-history-limit > 0") - } - image.Spec.FailedBuildHistoryLimit = &failedBuildHistoryLimit + image.Spec.FailedBuildHistoryLimit, err = f.makeBuildHistoryLimit(f.FailedBuildHistoryLimit) + if err != nil { + return nil, err } return image, nil @@ -249,7 +242,7 @@ func (f *Factory) makeBuildHistoryLimit(buildHistoryLimit string) (*int64, error value, err := strconv.ParseInt(buildHistoryLimit, 10, 64) if err != nil || value < 1 { - return nil, errors.New("must provide a valid build history limit >1") + return nil, errors.New("must provide a valid build history limit > 0") } return &value, nil diff --git a/pkg/image/factory_test.go b/pkg/image/factory_test.go index 72ef6ac..f855020 100644 --- a/pkg/image/factory_test.go +++ b/pkg/image/factory_test.go @@ -86,7 +86,7 @@ func testImageFactory(t *testing.T, when spec.G, it spec.S) { factory.FailedBuildHistoryLimit = "10" factory.Blob = "some-blob" _, err := factory.MakeImage("test-name", "test-namespace", "test-registry.io/test-image") - require.EqualError(t, err, "must provide a valid success-build-history-limit > 0") + require.EqualError(t, err, "must provide a valid build history limit > 0") }) }) diff --git a/pkg/image/update_factory.go b/pkg/image/update_factory.go index 0cffab1..08cf8af 100644 --- a/pkg/image/update_factory.go +++ b/pkg/image/update_factory.go @@ -45,6 +45,10 @@ func (f *Factory) UpdateImage(img *v1alpha2.Image) (*v1alpha2.Image, error) { f.setBuilder(updatedImage) f.setServiceAccount(updatedImage) + err = f.setBuildHistoryLimit(updatedImage) + if err != nil { + return nil, err + } return updatedImage, nil } @@ -345,3 +349,21 @@ func (f *Factory) setServiceAccount(image *v1alpha2.Image) { image.Spec.ServiceAccountName = f.ServiceAccount } } + +func (f *Factory) setBuildHistoryLimit(image *v1alpha2.Image) error { + var err error + if f.SuccessBuildHistoryLimit != "" { + image.Spec.SuccessBuildHistoryLimit, err = f.makeBuildHistoryLimit(f.SuccessBuildHistoryLimit) + if err != nil { + return err + } + } + + if f.FailedBuildHistoryLimit != "" { + image.Spec.FailedBuildHistoryLimit, err = f.makeBuildHistoryLimit(f.FailedBuildHistoryLimit) + if err != nil { + return err + } + } + return nil +}