Feedback on various issues

[skeeto]

You should guard against file names that look like options:

cmd := exec.Command("gpg", "--decrypt", filePath)

If filePath happened to be --sign then it would be accepted as an option and you'd get the wrong results, possibly silently. Put a -- before the file name to stop option processing:

cmd := exec.Command("gpg", "--decrypt", "--", filePath)

There's a bytes.NewReader so you don't need to make an unneeded, extra copy of the user's data in memory:

var out []byte
// ...
r := strings.NewReader(string(out))

However, even better would be to stream the output of GnuPG straight into the bucket. The minio interface already takes a reader, so you could just hook up its StdoutPipe(). Set the length to -1.

Same goes for decryption: Connect minio to the GnuPG process like a pipe. This avoids the need for (badly and dangerously) creating temporary files.

Strongly consider explicitly using GnuPG's "unattended" interface via --batch and maybe --status-fd and --with-colons. That's how GnuPG is intended to be run non-interactively by other programs.

Silence is golden. Do you really need to print output when everything was successful? Consider how go build itself prints nothing on success.

panic is not an appropriate way to deal with everyday errors. Outside of OOM, regular users should never see panic messages. If PutObject fails (e.g. network error), print a useful error message. (IMHO, also it would also be better style to pass this error up to the top level, or nearly so, and let it do the printing, rather than mingling UI with your business logic. Same goes for validating command line options.)

Always exit the process with an error status when an error terminates processing. Also, check the error results! Currently there a number of ways the program may silently lose data.

Never print errors on standard output, especially if the user is expecting their data on standard output. Combined with the lack of error exit status, this is a recipe for disaster. If something fails, the user will never see the error, nor have any way to know it failed. Instead they'll quietly have the wrong data. This is a programming cardinal sin.

Get more comfortable with byte slices, []byte. There are lots of needless, and even incorrect, string conversions. For instance:

fmt.Println(string(data))

Just do this instead:

os.Stdout.Write(data)

Not only is it more efficient, it doesn't append a spurious newline to the user's data. Also check the error!

[skeeto]

To illustrate some of these things, here's what I mean about passing errors back up to the top level:

diff --git a/cmd/encrypt.go b/cmd/encrypt.go
index 5ab9863..4143a93 100644
--- a/cmd/encrypt.go
+++ b/cmd/encrypt.go
@@ -26,12 +26,16 @@ var encCommand = &cobra.Command{
                note, _ := cmd.Flags().GetString("note")
                file, _ := cmd.Flags().GetString("file")
                isPrint, _ := cmd.Flags().GetBool("print")
+               var err error
                if note != "" {
-                       encryptString(note, isPrint)
+                       err = encryptString(note, isPrint)
                }
 
                if file != "" {
-                       encryptFile(file, isPrint)
+                       err = encryptFile(file, isPrint)
+               }
+               if err != nil {
+                       log.Fatal(err)
                }
        },
 }
@@ -47,7 +51,7 @@ func init() {
        encCommand.Flags().BoolP("print", "p", false, "-p")
 }
 
-func encryptString(value string, isPrint bool) {
+func encryptString(value string, isPrint bool) error {
        cmd := exec.Command("gpg", "--encrypt", "-r", gpgID, "--armor")
 
        isDone, _ := pterm.DefaultSpinner.Start()
@@ -110,7 +114,7 @@ func encryptString(value string, isPrint bool) {
        defer isDone.Success("Successfully encrypted and saved! ", status)
 }
 
-func encryptFile(filePath string, isPrint bool) {
+func encryptFile(filePath string, isPrint bool) error {
        cmd := exec.Command("gpg", "--encrypt", "--armor", "-r", gpgID, "-o", "/dev/stdout", filePath)
 
        out, err := cmd.Output()

Then you can simply return err in these functions any time you have a non-nil error. No more panic.

For piping to the bucket, that looks something like this:

--- a/cmd/encrypt.go
+++ b/cmd/encrypt.go
@@ -113,32 +113,40 @@ func encryptString(value string, isPrint bool) {
 func encryptFile(filePath string, isPrint bool) error {
 	cmd := exec.Command("gpg", "--encrypt", "--armor", "-r", gpgID, "-o", "/dev/stdout", filePath)
 
-	out, err := cmd.Output()
+	var err error
+	var stdout io.Reader
+	if stdout, err = cmd.StdoutPipe(); err != nil {
+		return err
+	}
 
+	err = cmd.Start()
 	if err != nil {
-		log.Fatal(err)
+		return err
 	}
+	defer cmd.Wait()
 
-	result := string(out)
 	isDone, _ := pterm.DefaultSpinner.Start()
 
 	if isPrint {
-		fmt.Println(result)
-		return
+		stdout = io.TeeReader(stdout, os.Stdout)
 	}
 
-	readedResult := strings.NewReader(result)
 	fileName := filepath.Base(filePath)
 
 	bucketName := os.Getenv("MINIO_BUCKET_NAME")
 
-	status, err := Client.PutObject(bucketName, "/files/"+fileName+".asc", readedResult, int64(len(result)), minio.PutObjectOptions{
+	status, err := Client.PutObject(bucketName, "/files/"+fileName+".asc", stdout, -1, minio.PutObjectOptions{
 		ContentType: "application/pgp-encrypted",
 	})
 
 	if err != nil {
-		panic(err)
+		return err
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return err
 	}
 
 	isDone.Success("Successfully encrypted and saved! ", status)
+	return nil
 }

[jack5341]

Thank you for the your valuable feedback. If you want to be contributer you can directly create a PR and i'll happy to accept your PR.