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.

Sign up for GitHub

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

Put os.Stdin size to NewReaderSize instead NewReader #441

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Put os.Stdin size to NewReaderSize instead NewReader #441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db768bd
fix buff size
IgorHalfeld Jul 9, 2020
de18f39
better if/else block and fix code coverage
IgorHalfeld Jul 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 31 additions & 21 deletions cmd/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package cmd
import (
"bufio"
"fmt"
"io/ioutil"
"io"
"os"
"strings"

Expand Down Expand Up @@ -31,8 +31,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {
}

listCommand := &cobra.Command{
Short: "List all contexts",
Use: "list <vcs-type> <org-name>",
Short: "List all contexts",
Use: "list <vcs-type> <org-name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return listContexts(cl, args[0], args[1])
Expand All @@ -41,8 +41,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {
}

showContextCommand := &cobra.Command{
Short: "Show a context",
Use: "show <vcs-type> <org-name> <context-name>",
Short: "Show a context",
Use: "show <vcs-type> <org-name> <context-name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return showContext(cl, args[0], args[1], args[2])
Expand All @@ -51,8 +51,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {
}

storeCommand := &cobra.Command{
Short: "Store a new environment variable in the named context. The value is read from stdin.",
Use: "store-secret <vcs-type> <org-name> <context-name> <secret name>",
Short: "Store a new environment variable in the named context. The value is read from stdin.",
Use: "store-secret <vcs-type> <org-name> <context-name> <secret name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return storeEnvVar(cl, args[0], args[1], args[2], args[3])
Expand All @@ -61,8 +61,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {
}

removeCommand := &cobra.Command{
Short: "Remove an environment variable from the named context",
Use: "remove-secret <vcs-type> <org-name> <context-name> <secret name>",
Short: "Remove an environment variable from the named context",
Use: "remove-secret <vcs-type> <org-name> <context-name> <secret name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return removeEnvVar(cl, args[0], args[1], args[2], args[3])
Expand All @@ -71,8 +71,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {
}

createContextCommand := &cobra.Command{
Short: "Create a new context",
Use: "create <vcs-type> <org-name> <context-name>",
Short: "Create a new context",
Use: "create <vcs-type> <org-name> <context-name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return createContext(cl, args[0], args[1], args[2])
Expand All @@ -82,8 +82,8 @@ func newContextCommand(config *settings.Config) *cobra.Command {

force := false
deleteContextCommand := &cobra.Command{
Short: "Delete the named context",
Use: "delete <vcs-type> <org-name> <context-name>",
Short: "Delete the named context",
Use: "delete <vcs-type> <org-name> <context-name>",
PreRunE: initClient,
RunE: func(cmd *cobra.Command, args []string) error {
return deleteContext(cl, force, args[0], args[1], args[2])
Expand Down Expand Up @@ -169,17 +169,27 @@ func showContext(client *client.Client, vcsType, orgName, contextName string) er
return nil
}

func readSecretValue() (string, error) {
// ReadSecretValue reads a secret from a buffer
func ReadSecretValue() (string, error) {
stat, _ := os.Stdin.Stat()

buffSize, err := os.Stdin.Stat()
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the redundancy, would it be possible to remove this Stat() call and instead use the one on line 174?

Suggested change
buffSize, err := os.Stdin.Stat()
stat, err := os.Stdin.Stat()
if err != nil {
return "", err
}
reader := bufio.NewReaderSize(os.Stdin, int(stat.Size()))
// ...

if err != nil {
return "", err
}

reader := bufio.NewReaderSize(os.Stdin, int(buffSize.Size()))

if (stat.Mode() & os.ModeCharDevice) == 0 {
bytes, err := ioutil.ReadAll(os.Stdin)
bytes := make([]byte, buffSize.Size())
_, err := io.ReadFull(reader, bytes)
Comment on lines -175 to +185
Copy link
Contributor

@kelvinkfli kelvinkfli Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Did you happen to experience errors other than failed to read input: bufio.Scanner: token too long from this code path?

I believe that ioutil.ReadAll will read the entire contents of the provided io.Reader without any limitations. It seems that '512' refers to the initial buffer size, which will dynamically grow until EOF/OOM occurs, so I'm not certain changing this to io.ReadFull will address that error specifically.

return string(bytes), err
} else {
fmt.Print("Enter secret value and press enter: ")
reader := bufio.NewReader(os.Stdin)
str, err := reader.ReadString('\n')
return strings.TrimRight(str, "\n"), err
}

fmt.Print("Enter secret value and press enter: ")

str, err := reader.ReadString('\n')
return strings.TrimRight(str, "\n"), err
}

func createContext(client *client.Client, vcsType, orgName, contextName string) error {
Expand All @@ -201,7 +211,7 @@ func storeEnvVar(client *client.Client, vcsType, orgName, contextName, varName s
if err != nil {
return err
}
secretValue, err := readSecretValue()
secretValue, err := ReadSecretValue()

if err != nil {
return errors.Wrap(err, "Failed to read secret value from stdin")
Expand Down
29 changes: 29 additions & 0 deletions cmd/context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package cmd_test

import (
"io/ioutil"
"log"
"os"
"os/exec"

"github.com/CircleCI-Public/circleci-cli/clitest"
"github.com/CircleCI-Public/circleci-cli/cmd"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gbytes"
Expand Down Expand Up @@ -36,6 +40,31 @@ You can create a new personal API token here:
https://circleci.com/account/api`))
Eventually(session).Should(clitest.ShouldFail())
})

It("ReadSecretValue should read correctly os.Stdin wihout given a error", func() {
By("running the command")
content := []byte("stdin mock!")
tempFile, err := ioutil.TempFile("", "stdin-mock")
if err != nil {
log.Fatalln(err)
}

if _, err := tempFile.Write(content); err != nil {
log.Fatal(err)
}

if _, err := tempFile.Seek(0, 0); err != nil {
log.Fatal(err)
}

defer os.Remove(tempFile.Name())

os.Stdin = tempFile
result, err := cmd.ReadSecretValue()

Expect(err).ShouldNot(HaveOccurred())
Expect(result).Should(Equal(string(content)))
})
})

// TODO: add integration tests for happy path cases
Expand Down