Build-206: BuildRun CRUD and logging #6

alicerum · 2021-03-29T12:54:14Z

This pull request contains new sub-commands for working with BuildRun objects.
BuildRun can be created via command build run.
They also can be deleted, listed and logs can be requested via buildrun logs
Resource objects now support context passed from cobra commands.
SubCommands Run method now accepts IoStreams for information output.
Other minor changes, including README.

alicerum · 2021-03-29T12:54:43Z

@otaviof could you please take a look at this?

coreydaley

Here is my first run through of this pull request.

coreydaley · 2021-03-29T14:21:15Z

cmd/shp/main.go

 	if err := rootCmd.Execute(); err != nil {
-		fmt.Printf("[ERROR] %#v\n", err)
+		fmt.Fprintf(os.Stderr, "ERROR: %v\n", err)


Can we switch to using klog v2 here (and throughout ShipWright)?
That would give us logging levels and InfoF, WarnF, ErrorF, etc.

I would suggest making a small issue for that. I think it must be done, but not as part of this PR.

Agree. Feel free to create issues and reference them here, please. :-)

#7 was created for this issue.

coreydaley · 2021-03-29T14:31:42Z

pkg/shp/cmd/build/delete.go

+	br := resource.GetBuildResource(params)
+	brr := resource.GetBuildRunResource(params)
+
+	if err := br.Get(c.cmd.Context(), c.name, &b); err != nil {


If you are just checking for the existence here before you run the delete, you shouldn't need to, the delete should return an error if it doesn't exist.

coreydaley · 2021-03-29T14:33:34Z

pkg/shp/cmd/build/delete.go

+	if c.deleteRuns {
+		label := fmt.Sprintf("build.build.dev/name=%v", c.name)
+		var brList buildv1alpha1.BuildRunList
+		err := brr.ListWithOptions(c.cmd.Context(), &brList, v1.ListOptions{


if err := brr.ListWithOptions(c.cmd.Context(), &brList, v1.ListOptions{ LabelSelector: label, }); err != nil { return err }

coreydaley · 2021-03-29T14:33:51Z

pkg/shp/cmd/build/delete.go

+			LabelSelector: label,
+		})
+
+		if err != nil {


Then you can get rid of this

coreydaley · 2021-03-29T14:34:18Z

pkg/shp/cmd/build/delete.go

+		}
+
+		for _, buildrun := range brList.Items {
+			brr.Delete(c.cmd.Context(), buildrun.Name)


Do we care about any errors that might happen here?

coreydaley · 2021-03-29T14:55:38Z

pkg/shp/cmd/build/create.go

+		return err
+	}
+
+	fmt.Fprintf(io.Out, "Build created \"%v\"\n", sc.name)


You can replace \"%v\" with %q which will give you a double-quoted string safely escaped with Go syntax

coreydaley · 2021-03-29T14:57:32Z

pkg/shp/cmd/build/delete.go

 }

 func deleteCmd() runner.SubCommand {
 	deleteCommand := &DeleteCommand{
 		cmd: &cobra.Command{
-			Use:   "delete [flags] name",
+			Use:   "delete [flags] [name]",


I don't think that [name] should have the square brackets around it (same with the other commands), unless you have a specific reason?

I wanted it to be in brackets, because unlike delete it's a variable, or a parameter, not subcommand.
But, I have just looked through man pages conventions, and it seems, that angular brackets (<>) might be a better choice here, because unlike square brackets ([]), which represent optional parameter or value, angular ones represent mandatory parameter or piece of data.

My suggestion is shp delete <name> [flags]. By using <> we delimitates the required items, which also should come first, and by using [] the optional -- as you've mentioned.

Do we agree with <> for required and [] for optional?

i like this format. and i think it is pretty self explanatory, so should work out well.

coreydaley · 2021-03-29T14:58:06Z

pkg/shp/cmd/build/delete.go

 			Short: "Delete Build",
 		},
 	}

+	deleteCommand.cmd.Flags().BoolVarP(&deleteCommand.deleteRuns, "delete-runs", "r", false, "Also delete all the buildruns")


s/all the/all of the/

coreydaley · 2021-03-29T14:59:26Z

pkg/shp/cmd/build/delete.go

 	var b buildv1alpha1.Build

-	if err := buildResource.Get(c.name, &b); err != nil {
+	br := resource.GetBuildResource(params)
+	brr := resource.GetBuildRunResource(params)


You are only using the brr variable inside of the if c.deleteRuns block, so you should probably just create it inside of that block, since the code may never even be executed if that option is not set.

coreydaley · 2021-03-29T15:02:48Z

pkg/shp/cmd/build/delete.go

 		return err
 	}

-	return buildResource.Delete(c.name)
+	if c.deleteRuns {
+		label := fmt.Sprintf("build.build.dev/name=%v", c.name)


It is probably safe to just replace the LabelSelector: label, with LabelSelector: fmt.Sprintf(...), instead of creating this separate variable.

TODO: I also had it changed to const from build, but it probably got lost during the extensive cherry pick after the previous PR

otaviof

Here it goes my initial review comments, please consider.

otaviof · 2021-03-30T08:49:07Z

br.yaml

@@ -0,0 +1,7 @@
+apiVersion: build.dev/v1alpha1


Is this file intentionally part of the PR? I looks like a left over.

yup, it's an accident from my latest testing, i'll remove it

otaviof · 2021-03-30T08:49:35Z

cmd/shp/main.go

 	if err := rootCmd.Execute(); err != nil {
-		fmt.Printf("[ERROR] %#v\n", err)
+		fmt.Fprintf(os.Stderr, "ERROR: %v\n", err)


Agree. Feel free to create issues and reference them here, please. :-)

otaviof · 2021-03-30T08:56:09Z

pkg/shp/cmd/build/create.go

@@ -28,7 +31,7 @@ type CreateCommand struct {
 func createCmd() runner.SubCommand {
 	createCommand := &CreateCommand{
 		cmd: &cobra.Command{
-			Use:   "create [flags] name strategy url",
+			Use:   "create [flags] [name] [strategy] [url]",


I think we should have URL and strategy as flags, so it highlights the attributes used. Also, we should consider having a default strategy, therefore when not informed we could assume "buildpacks" and have --strategy as optional.

shp create <name> --url=<url> --strategy=<strategy>

otaviof · 2021-03-30T08:59:01Z

pkg/shp/cmd/build/delete.go

 }

 func deleteCmd() runner.SubCommand {
 	deleteCommand := &DeleteCommand{
 		cmd: &cobra.Command{
-			Use:   "delete [flags] name",
+			Use:   "delete [flags] [name]",


My suggestion is shp delete <name> [flags]. By using <> we delimitates the required items, which also should come first, and by using [] the optional -- as you've mentioned.

Do we agree with <> for required and [] for optional?

otaviof · 2021-03-30T09:00:08Z

pkg/shp/cmd/build/run.go

+	"github.com/shipwright-io/cli/pkg/shp/resource"
+)
+
+type RunCommand struct {


Would be great do have doc-comments, and perhaps a initial doc.go 🙏🏼

alicerum · 2021-03-30T14:11:16Z

In the latest commit I've fixed most of the requested changes. I haven't commited anything regarding the buildrun logs yet. Doing some research and testing before I do that.

otaviof · 2021-03-31T11:13:03Z

/lgtm

Good changes, Alice! Thanks.

coreydaley · 2021-03-31T15:28:24Z

@alicerum If you would squash your two code commits I would like to take another run through it.

alicerum · 2021-03-31T15:34:46Z

@coreydaley done, thank you

coreydaley · 2021-03-31T16:47:04Z

pkg/shp/cmd/buildrun/common.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func NewBuildRun(build *buildv1alpha1.Build, prefix string) *buildv1alpha1.BuildRun {


Exported functions should have comments

coreydaley · 2021-03-31T16:49:21Z

The exported functions in this pull request, that are not boilerplate Cobra code, probably need comments added, at least a short blurb.

alicerum · 2021-04-01T09:10:47Z

@coreydaley done

coreydaley

Looks good to me

otaviof · 2021-04-06T08:48:47Z

/approve

New Subcommands for working with BuildRun Created via 'build run' Can be deleted, listed and view logs Resource objects now support context passed from cobra commands SubCommand Run method now accepts IoStreams for information output Other minor changes, including README

openshift-ci-robot · 2021-04-07T14:19:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: otaviof

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [otaviof]

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

adambkaplan · 2021-04-07T14:20:28Z

/refresh

adambkaplan

/lgtm

adambkaplan · 2021-04-07T18:40:45Z

Need to determine why tide is not working with the cli repo. Going to use the "green button"

alicerum requested review from otaviof and coreydaley March 29, 2021 12:54

coreydaley suggested changes Mar 29, 2021

View reviewed changes

otaviof reviewed Mar 30, 2021

View reviewed changes

alicerum mentioned this pull request Mar 30, 2021

Support verbose/granular output levels #7

Closed

coreydaley assigned otaviof and coreydaley Mar 30, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021

alicerum force-pushed the build-206 branch from e28ea09 to af74e8e Compare March 31, 2021 15:34

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021

coreydaley reviewed Mar 31, 2021

View reviewed changes

alicerum force-pushed the build-206 branch from af74e8e to eabfe51 Compare April 1, 2021 09:06

coreydaley approved these changes Apr 1, 2021

View reviewed changes

otaviof approved these changes Apr 6, 2021

View reviewed changes

Alice Rum added 2 commits April 7, 2021 16:19

bump: vendor update

47e2250

alicerum force-pushed the build-206 branch from eabfe51 to 863e038 Compare April 7, 2021 14:19

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2021

adambkaplan approved these changes Apr 7, 2021

View reviewed changes

adambkaplan merged commit 1d887fb into shipwright-io:main Apr 7, 2021

openshift-ci-robot assigned adambkaplan Apr 7, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

Build-206: BuildRun CRUD and logging #6

Build-206: BuildRun CRUD and logging #6

Conversation

alicerum commented Mar 29, 2021

alicerum commented Mar 29, 2021

coreydaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicerum commented Mar 30, 2021

otaviof commented Mar 31, 2021

coreydaley commented Mar 31, 2021

alicerum commented Mar 31, 2021

Choose a reason for hiding this comment

coreydaley commented Mar 31, 2021

alicerum commented Apr 1, 2021

coreydaley left a comment

Choose a reason for hiding this comment

otaviof commented Apr 6, 2021

openshift-ci-robot commented Apr 7, 2021

adambkaplan commented Apr 7, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan commented Apr 7, 2021