add cli changes to move to v1beta1 by karanibm6 · Pull Request #304 · shipwright-io/cli

karanibm6 · 2025-03-24T06:28:31Z

Changes

Add code changes to move to v1beta1 from v1alpha1

Fixes #294

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

The CLI now interacts with the Shipwright artifacts using the v1beta1 API

HeavyWombat

Thanks for all the work. Nice. I just left some comments and questions after an initial pass.

HeavyWombat · 2025-03-24T13:48:53Z

docs/shp_build_create.md

+      --source-oci-artifact-image string         source oci artifact image location, e.g. ghcr.io/shipwright-io/sample-go/source-bundle:latest
+      --source-oci-artifact-prune pruneOption    source oci artifact image prune option, either Never, or AfterPull (default Never)


Nit: I think I should try uppercase since it's an acronym.

Suggested change

--source-oci-artifact-image string source oci artifact image location, e.g. ghcr.io/shipwright-io/sample-go/source-bundle:latest

--source-oci-artifact-prune pruneOption source oci artifact image prune option, either Never, or AfterPull (default Never)

--source-oci-artifact-image string source OCI artifact image location, e.g. ghcr.io/shipwright-io/sample-go/source-bundle:latest

--source-oci-artifact-prune pruneOption source OCI artifact image prune option, either Never, or AfterPull (default Never)

I believe this doc is auto-generated, so it needs to be made in the respective go code.

HeavyWombat · 2025-03-24T13:50:32Z

pkg/shp/cmd/build/create.go

+	buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
 	"github.com/spf13/cobra"

+	v1 "k8s.io/api/core/v1"


For consistency, we should use "full name".

Suggested change

v1 "k8s.io/api/core/v1"

corev1 "k8s.io/api/core/v1"

HeavyWombat · 2025-03-24T13:54:45Z

pkg/shp/flags/build.go

+			Git: &buildv1beta1.Git{
+				Revision:    ptr.To(""),
+				CloneSecret: ptr.To(""),
+			},
+			OCIArtifact: &buildv1beta1.OCIArtifact{
+				Prune:      &pruneOption,
+				PullSecret: ptr.To(""),


Seems a bit a weird to me to initialize everything with pointers to empty strings just to be later updated by flag values, if I understand it correctly. Is there any way we can simplify this? What do you think?

I think we can take this up as a follow-up item to address. Since this PR is a pure refactor, I'd prefer to keep the existing behavior as close as possible to status quo.

Filed #310 so we don't lose track of this.

adambkaplan

/approve

Giving the 👍 for this change to go into v0.16. I called out a few items I noticed in my review.

adambkaplan · 2025-03-26T14:39:09Z

docs/shp_build_create.md

+      --source-oci-artifact-image string         source oci artifact image location, e.g. ghcr.io/shipwright-io/sample-go/source-bundle:latest
+      --source-oci-artifact-prune pruneOption    source oci artifact image prune option, either Never, or AfterPull (default Never)


I believe this doc is auto-generated, so it needs to be made in the respective go code.

pkg/shp/cmd/build/create.go

adambkaplan · 2025-03-26T14:49:15Z

pkg/shp/cmd/buildrun/create.go

 		return err
 	}
-	fmt.Fprintf(ioStreams.Out, "BuildRun created %q for Build %q\n", c.name, br.Spec.BuildRef.Name)
+	fmt.Fprintf(ioStreams.Out, "BuildRun created %q for Build %q\n", c.name, *br.Spec.Build.Name)


Risk of nil pointer dereference panic?

Looks like the nil dereference risk is still here...

I think in this context, we never have a standalone buildrun but always one that references a build? If so, would be worth a comment.

adambkaplan · 2025-03-26T14:53:19Z

pkg/shp/flags/build.go

+			Git: &buildv1beta1.Git{
+				Revision:    ptr.To(""),
+				CloneSecret: ptr.To(""),
+			},
+			OCIArtifact: &buildv1beta1.OCIArtifact{
+				Prune:      &pruneOption,
+				PullSecret: ptr.To(""),


I think we can take this up as a follow-up item to address. Since this PR is a pure refactor, I'd prefer to keep the existing behavior as close as possible to status quo.

pkg/shp/flags/flags.go

openshift-ci · 2025-03-26T16:33:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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~~ [adambkaplan]

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

adambkaplan · 2025-04-22T13:08:11Z

pkg/shp/cmd/build/create.go

+		}
+	}
+
+	if c.dockerfile != nil && *c.dockerfile != "" {


nit (not going to block on this) - you can do both checks in golang using len, since it is safe to use on nil pointers:

Suggested change

if c.dockerfile != nil && *c.dockerfile != "" {

if len(c.dockerfile) > 0 {

@adambkaplan , I know it works with arrays, but I tried this : https://go.dev/play/p/_nEyrfl96EZ and it didn't work for pointer strings.

adambkaplan · 2025-04-22T13:09:03Z

pkg/shp/cmd/build/create.go

+		c.buildSpec.ParamValues = append(c.buildSpec.ParamValues, dockerfileParam)
+	}
+
+	if c.builderImage != nil && *c.builderImage != "" {


nit (not blocking) - ditto on using len:

Suggested change

if c.builderImage != nil && *c.builderImage != "" {

if len(c.builderImage) > 0 {

@adambkaplan , I know it works with arrays, but I tried this : https://go.dev/play/p/_nEyrfl96EZ and it didn't work for pointer strings.

adambkaplan · 2025-04-22T13:10:00Z

pkg/shp/bundle/bundle.go


-	if buildRun.Spec.BuildRef != nil {
-		name, namespace := buildRun.Spec.BuildRef.Name, buildRun.Namespace
+	if buildRun.Spec.Build.Name != nil && *buildRun.Spec.Build.Name != "" {


nit - ditto on use of len:

Suggested change

if buildRun.Spec.Build.Name != nil && *buildRun.Spec.Build.Name != "" {

if len(buildRun.Spec.Build.Name) > 0 {

@adambkaplan , I know it works with arrays, but I tried this : https://go.dev/play/p/_nEyrfl96EZ and it didn't work for pointer strings.

adambkaplan · 2025-04-22T13:21:59Z

pkg/shp/flags/flags.go

 		false,
 		"generate a Kubernetes service-account for the build",
 	)
+	flags.MarkDeprecated("sa-generate", fmt.Sprintf("this flag has no effect, please use --%s for service account", ServiceAccountNameFlag))


Note that with this change, we should also remove the service account generation logic in the BuildRun controller. Filed shipwright-io/build#1873

So, we don't need any change in cli. Right ?

adambkaplan · 2025-04-22T13:35:45Z

@karanibm6 almost there! I found one item that was missed in your updates, plus a few nits that otherwise wouldn't block merge. Once these are done, please squash your commits and then this should be ready to merge 😄 !

SaschaSchwarze0

Nice work.

test/stub/stub.go

SaschaSchwarze0 · 2025-04-27T14:32:23Z

pkg/shp/cmd/buildrun/create.go

 		return err
 	}
-	fmt.Fprintf(ioStreams.Out, "BuildRun created %q for Build %q\n", c.name, br.Spec.BuildRef.Name)
+	fmt.Fprintf(ioStreams.Out, "BuildRun created %q for Build %q\n", c.name, *br.Spec.Build.Name)


I think in this context, we never have a standalone buildrun but always one that references a build? If so, would be worth a comment.

pkg/shp/flags/build.go

Signed-off-by: Karan Kumar <karan.kumar6@ibm.com>

SaschaSchwarze0

/lgtm

openshift-ci bot added the release-note label Mar 24, 2025

pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 24, 2025

shipwright-ci-bot added this to Shipwright Overview Mar 24, 2025

openshift-ci bot requested review from HeavyWombat and qu1queee March 24, 2025 06:28

HeavyWombat requested changes Mar 24, 2025

View reviewed changes

HeavyWombat requested a review from SaschaSchwarze0 March 24, 2025 13:57

karanibm6 mentioned this pull request Mar 24, 2025

[FEATURE] Use v1beta1 APIs Only #294

Closed

1 task

adambkaplan reviewed Mar 26, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2025

karanibm6 force-pushed the update-cli-params branch 2 times, most recently from 8320cbf to 74ec843 Compare April 7, 2025 03:43

pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2025

karanibm6 force-pushed the update-cli-params branch 5 times, most recently from 8cb2147 to cfea747 Compare April 7, 2025 12:59

karanibm6 requested review from HeavyWombat and adambkaplan April 7, 2025 18:05

karanibm6 force-pushed the update-cli-params branch from e051935 to 359459c Compare April 7, 2025 18:24

This was referenced Apr 21, 2025

add nodeSelector flag (--node) using v1beta1 rxinui/shipwright-cli#2

Closed

Add --node-selector Flag for shp build/buildrun Commands #309

Merged

adambkaplan mentioned this pull request Apr 22, 2025

[FEATURE] Remove Service Account Generation Feature shipwright-io/build#1873

Open

1 task

adambkaplan reviewed Apr 22, 2025

View reviewed changes

adambkaplan mentioned this pull request Apr 22, 2025

[TECH-DEBT] Use Empty BuildSpec in BuildSpecFromFlags method #310

Open

karanibm6 force-pushed the update-cli-params branch from 359459c to fa0e537 Compare April 24, 2025 13:50

karanibm6 requested a review from adambkaplan April 24, 2025 13:50

This was referenced Apr 24, 2025

Ship 0039/261 - add --scheduler-name flag #311

Merged

[BUG] v1alpha1 Build Strategies with builder-image parameters fail shipwright-io/build#1431

Open

SaschaSchwarze0 requested changes Apr 27, 2025

View reviewed changes

add cli changes to move to v1beta1 from v1alpha1

0afb42b

Signed-off-by: Karan Kumar <karan.kumar6@ibm.com>

karanibm6 force-pushed the update-cli-params branch from fa0e537 to 0afb42b Compare April 28, 2025 09:18

karanibm6 requested a review from SaschaSchwarze0 April 28, 2025 09:18

SaschaSchwarze0 approved these changes Apr 28, 2025

View reviewed changes

openshift-ci bot assigned SaschaSchwarze0 Apr 28, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2025

SaschaSchwarze0 added this to the release-v0.16.0 milestone Apr 28, 2025

SaschaSchwarze0 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 28, 2025

openshift-merge-bot bot merged commit 0fe28ba into shipwright-io:main Apr 28, 2025
10 checks passed

github-project-automation bot moved this to Done in Shipwright Overview Apr 28, 2025

SaschaSchwarze0 mentioned this pull request Jun 16, 2025

Blog post for v0.16.0 shipwright-io/website#172

Merged

3 tasks

		--source-oci-artifact-image string source oci artifact image location, e.g. ghcr.io/shipwright-io/sample-go/source-bundle:latest
		--source-oci-artifact-prune pruneOption source oci artifact image prune option, either Never, or AfterPull (default Never)

	if c.dockerfile != nil && *c.dockerfile != "" {
	if len(c.dockerfile) > 0 {

	if c.builderImage != nil && *c.builderImage != "" {
	if len(c.builderImage) > 0 {

	if buildRun.Spec.Build.Name != nil && *buildRun.Spec.Build.Name != "" {
	if len(buildRun.Spec.Build.Name) > 0 {

Conversation

karanibm6 commented Mar 24, 2025 • edited by SaschaSchwarze0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

HeavyWombat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Apr 22, 2025

Uh oh!

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

karanibm6 commented Mar 24, 2025 •

edited by SaschaSchwarze0

Loading