Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add package push command #340

Merged
merged 11 commits into from
Sep 3, 2024
2 changes: 1 addition & 1 deletion cmd/bk/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 1
}

Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ require (
github.com/sourcegraph/conc v0.3.0 // indirect
github.com/spf13/afero v1.11.0
github.com/spf13/cast v1.6.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/spf13/pflag v1.0.5
github.com/subosito/gotenv v1.6.0 // indirect
github.com/suessflorian/gqlfetch v0.6.0
github.com/xanzy/ssh-agent v0.3.3 // indirect
Expand Down
19 changes: 19 additions & 0 deletions pkg/cmd/pkg/package.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package pkg

import (
"github.com/buildkite/cli/v3/pkg/cmd/factory"
"github.com/buildkite/cli/v3/pkg/cmd/validation"
"github.com/spf13/cobra"
)

func NewCmdPackage(f *factory.Factory) *cobra.Command {
cmd := cobra.Command{
Use: "package <command>",
Short: "Manage packages",
Long: "Work with Buildkite Packages",
PersistentPreRunE: validation.CheckValidConfiguration(f.Config),
}

cmd.AddCommand(NewCmdPackagePush(f))
return &cmd
}
164 changes: 164 additions & 0 deletions pkg/cmd/pkg/push.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package pkg

import (
"errors"
"fmt"
"io"
"os"

"github.com/MakeNowJust/heredoc"
"github.com/buildkite/cli/v3/pkg/cmd/factory"
"github.com/buildkite/go-buildkite/v3/buildkite"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

type pushPackageConfig struct {
RegistrySlug string
FilePath string
StdinFileName string
}

const stdinFileNameFlag = "stdin-file-name"

func NewCmdPackagePush(f *factory.Factory) *cobra.Command {
cmd := cobra.Command{
Use: "push registry-name {path/to/file | --stdin-file-name filename -}",
Short: "Push a new package to Buildkite Packages",
Long: heredoc.Doc(`
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.`),
Example: heredoc.Doc(`
$ 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
`),
RunE: func(cmd *cobra.Command, args []string) error {
cfg, err := loadAndValidateConfig(cmd.Flags(), args)
if err != nil {
return fmt.Errorf("failed to validate flags and args: %w", err)
}

var (
from io.Reader
packageName string
)

switch {
case cfg.FilePath != "":
packageName = cfg.FilePath

file, err := os.Open(cfg.FilePath)
if err != nil {
return fmt.Errorf("couldn't open file %s: %w", cfg.FilePath, err)
}
defer file.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh yup, good call

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


from = file
case cfg.StdinFileName != "":
packageName = cfg.StdinFileName
from = cmd.InOrStdin()

default:
panic("Neither file path nor stdin file name are available, there has been an error in the config validation. Report this to support@buildkite.com")
}

pkg, _, err := f.RestAPIClient.PackagesService.Create(f.Config.OrganizationSlug(), cfg.RegistrySlug, buildkite.CreatePackageInput{
Filename: packageName,
Package: from,
})
Comment on lines +66 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@moskyb moskyb Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return fmt.Errorf("%w: request to create package failed: %w", ErrAPIError, err)
}

fmt.Fprintf(cmd.OutOrStdout(), "Created package: %s\n", pkg.Name)
fmt.Fprintf(cmd.OutOrStdout(), "View it on the web at: %s\n", pkg.WebURL)
return nil
},
}

cmd.Flags().StringP(stdinFileNameFlag, "n", "", "The filename to use for the package, if it's passed via stdin. Invalid otherwise.")

return &cmd
}

var (
ErrInvalidConfig = errors.New("invalid config")
ErrAPIError = errors.New("API error")

// To be overridden in testing
// Actually diddling an io.Reader so that it looks like a readable stdin is tricky
// so we'll just stub this out
isStdInReadableFunc = isStdinReadable
)

func isStdinReadable() (bool, error) {
stat, err := os.Stdin.Stat()
if err != nil {
return false, fmt.Errorf("failed to stat stdin: %w", err)
}

readable := (stat.Mode() & os.ModeCharDevice) == 0
return readable, nil
}

func loadAndValidateConfig(flags *pflag.FlagSet, args []string) (*pushPackageConfig, error) {
stdinFileName := flags.Lookup(stdinFileNameFlag)
if stdinFileName == nil {
// This should never happen, as we're setting the flag in NewCmdPackagePush.
// Seeing this panic indicates a bug in the code.
panic(fmt.Sprintf("%s flag not found", stdinFileNameFlag))
}

if len(args) != 2 {
errS := fmt.Sprintf("Exactly 2 arguments are required, got: %d", len(args))
if stdinFileName.Value.String() != "" {
errS += " (when passing packages via stdin, the final argument must be '-')"
}
return nil, fmt.Errorf("%w: %s", ErrInvalidConfig, errS)
}

if args[1] == "-" && stdinFileName.Value.String() == "" {
return nil, fmt.Errorf("%w: When passing a package via stdin, the --stdin-file-name flag must be provided", ErrInvalidConfig)
}

if stdinFileName.Value.String() != "" {
if args[1] != "-" {
return nil, fmt.Errorf("%w: When passing a package via stdin, the final argument must be '-'", ErrInvalidConfig)
}

stdInReadable, err := isStdInReadableFunc()
if err != nil {
return nil, fmt.Errorf("failed to check if stdin is readable: %w", err)
}

if !stdInReadable {
return nil, fmt.Errorf("%w: stdin is not readable", ErrInvalidConfig)
}

return &pushPackageConfig{
RegistrySlug: args[0],
StdinFileName: stdinFileName.Value.String(),
}, nil
} else {
// No stdin file name, so we expect a file path as the second argument
filePath := args[1]
fi, err := os.Stat(filePath)
if err != nil {
return nil, fmt.Errorf("%w: %w", ErrInvalidConfig, err)
}

if !fi.Mode().IsRegular() {
mode := "directory"
if !fi.Mode().IsDir() {
mode = fi.Mode().String()
}
return nil, fmt.Errorf("%w: file at %s is not a regular file, mode was: %s", ErrInvalidConfig, filePath, mode)
}

return &pushPackageConfig{
RegistrySlug: args[0],
FilePath: filePath,
}, nil
}
}
Loading