Add package push command #340

moskyb · 2024-08-23T01:07:31Z

This PR: Adds a new command to the buildkite cli, package push. This command allows users to publish packages to buildkite packages using the CLI.

Attached is the help text for the new command:

Push a new package to Buildkite Packages. The package can be passed as a path to a file in the second positional argument,
or via stdin. If passed via stdin, the filename must be provided with the --stdin-file-name flag, as Buildkite
Packages requires a filename for the package.

Usage:
  bk package push registry-name {path/to/file | --stdin-file-name filename -} [flags]

Examples:
$ bk package push my-registry my-package.tar.gz
$ cat my-package.tar.gz | bk package push my-registry --stdin-file-name my-package.tar.gz - # Pass package via stdin, note hyphen as the argument


Flags:
  -h, --help                     help for push
  -n, --stdin-file-name string   The filename to use for the package, if it's passed via stdin. Invalid otherwise.

A usage example:

$ bkcli package push bennos-gemmos ./test-1.0.0.gem
Created package: test
View it on the web at: http://buildkite.localhost/organizations/buildkite/packages/registries/bennos-gemmos/packages/01918cd0-2568-73aa-8ea3-3d4ec8c0e615

jradtilbrook

This is very cool to see packages making its way into the CLI 💪

I will defer to you with more go experience, but have noted below lots of different ways than what we currently have for other commands if you can comment on those

pkg/cmd/pkg/package.go

pkg/cmd/pkg/push.go

jradtilbrook · 2024-08-23T05:15:57Z

pkg/cmd/pkg/push.go

+
+func NewCmdPackagePush(f *factory.Factory) *cobra.Command {
+	cmd := cobra.Command{
+		Use:   "push --registry <registry> {--file <file> | --file-name <filename> -}",


One other thing we've tried to do is allow each command to do "something" useful by default without any flags. And so all flags are optional to modify the default behaviour.

If there is something that is required, I think it should be an argument instead? Ie. should registry (or maybe both flags) be positional arguments instead?

yeah, i think that the right approach here is probably:

--registry becomes the first positional arg

--file becomes an optional second positional arg

--file-name gets renamed to --stdin-file-name, and allows the second positional arg to be omitted

jradtilbrook · 2024-08-26T04:10:45Z

go.mod

@@ -24,6 +24,8 @@ require (
 	golang.org/x/sync v0.7.0
 )

+replace github.com/buildkite/go-buildkite/v3 => ../go-buildkite


probably need to do another release and reverse this?

pkg/cmd/pkg/push.go

mcncl · 2024-08-27T02:02:41Z

pkg/cmd/pkg/push.go

+			pkg, _, err := f.RestAPIClient.PackagesService.Create(f.Config.OrganizationSlug(), cfg.RegistrySlug, buildkite.CreatePackageInput{
+				Filename: packageName,
+				Package:  from,
+			})


Should we add a check to ensure this isn't a nil pointer?

yeah, i was wondering about that earlier... theoretically loadAndValidateConfig should ensure that one or the other of the two sources is selected, but it can't hurt to double check.

given that neither stdin nor a file being available is likely an error in programming, i might add a

default: panic("oh no!")

to the switch block above.

Is this upload where the bulk of the time will be spent? Ie if its uploading a few MB or many more, it could take a while. We have a spinner package we use to give the user some feedback at least. I made a patch you can apply if you like: https://gist.github.com/jradtilbrook/a3af75bab23d26edb6f2bb2291bf17d1

I'm assuming simply opening the file won't take long and not worth including in the spinner

mcncl · 2024-08-27T02:03:16Z

pkg/cmd/pkg/push.go

+					return fmt.Errorf("couldn't open file %s: %w", cfg.FilePath, err)
+				}
+
+				defer file.Close()


Should this come before the error check to ensure the file is closed even in the result of an error?

ooh yup, good call

If os.Open returns an error, then file is (probably) nil, so defer file.Close() before checking err would probably cause a nil dereference panic.

TODO: Update to an actual version of go-buildkite

jradtilbrook · 2024-08-28T00:28:01Z

cmd/bk/main.go

@@ -30,7 +30,7 @@ func mainRun() int {
 	}

 	if err := rootCmd.ExecuteContext(ctx); err != nil {
-		fmt.Fprintf(os.Stderr, "failed to execute command: %s\n", err)
+		// Errors already get printed by the cobra command runner


Oh my god. That has been bugging me but I keep forgetting to create a ticket/look into it. Thank you!!

moskyb requested review from jradtilbrook, mcncl, lizrabuya and PriyaSudip as code owners August 23, 2024 01:07

jradtilbrook reviewed Aug 23, 2024

View reviewed changes

pkg/cmd/pkg/package.go Outdated Show resolved Hide resolved

pkg/cmd/pkg/push.go Outdated Show resolved Hide resolved

pkg/cmd/pkg/push.go Outdated Show resolved Hide resolved

pkg/cmd/pkg/push.go Outdated Show resolved Hide resolved

pkg/cmd/pkg/push.go Outdated Show resolved Hide resolved

jradtilbrook reviewed Aug 23, 2024

View reviewed changes

moskyb force-pushed the package-create branch 5 times, most recently from 43091dd to 677efcc Compare August 26, 2024 03:51

moskyb requested a review from jradtilbrook August 26, 2024 03:52

jradtilbrook reviewed Aug 26, 2024

View reviewed changes

pkg/cmd/pkg/push.go Outdated Show resolved Hide resolved

moskyb force-pushed the package-create branch from aaa683a to cab6e66 Compare August 26, 2024 23:37

jradtilbrook previously approved these changes Aug 27, 2024

View reviewed changes

moskyb dismissed jradtilbrook’s stale review via 65f02be August 27, 2024 01:12

moskyb force-pushed the package-create branch 3 times, most recently from 330ee8b to 96312c1 Compare August 27, 2024 01:56

mcncl reviewed Aug 27, 2024

View reviewed changes

moskyb force-pushed the package-create branch from 96312c1 to a83add3 Compare August 27, 2024 02:04

moskyb added 8 commits August 27, 2024 16:33

Add package push command

3a6c582

Refactor package push to use positional args over flags

64a0d69

Print out the package's name and URL after it's created

c10fb36

TODO: Update to an actual version of go-buildkite

Update go-buildkite to v3.13

bfcfef0

Remove unnecessary pipe + goroutine malarkey

480a6e8

Improve argument count error message when passing package file via stdin

d80b0ad

Add config error for forgetting the flag when using stdin

36fbcb2

Don't double-print errors when they occur

76f1ef0

Panic to ensure that there's an io.Reader to read the package from

ef77f43

moskyb force-pushed the package-create branch from 90f1e6b to ef77f43 Compare August 27, 2024 04:34

jradtilbrook reviewed Aug 28, 2024

View reviewed changes

jradtilbrook added 2 commits September 3, 2024 11:08

Merge branch '3.x' into package-create

9c20a59

Remove test gem file

43fd2b8

jradtilbrook enabled auto-merge (squash) September 3, 2024 03:14

jradtilbrook approved these changes Sep 3, 2024

View reviewed changes

PriyaSudip approved these changes Sep 3, 2024

View reviewed changes

jradtilbrook merged commit c01e962 into 3.x Sep 3, 2024
1 check passed

jradtilbrook deleted the package-create branch September 3, 2024 03:20

mcncl approved these changes Sep 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package push command #340

Add package push command #340

moskyb commented Aug 23, 2024 •

edited

Loading

jradtilbrook left a comment

jradtilbrook Aug 23, 2024

moskyb Aug 25, 2024

jradtilbrook Aug 26, 2024

mcncl Aug 27, 2024

moskyb Aug 27, 2024 •

edited

Loading

jradtilbrook Aug 28, 2024

mcncl Aug 27, 2024

moskyb Aug 27, 2024

DrJosh9000 Aug 27, 2024

jradtilbrook Aug 28, 2024

Add package push command #340

Add package push command #340

Conversation

moskyb commented Aug 23, 2024 • edited Loading

jradtilbrook 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

moskyb Aug 27, 2024 • edited Loading

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

moskyb commented Aug 23, 2024 •

edited

Loading

moskyb Aug 27, 2024 •

edited

Loading