mr un/approve: If -F, un/approve completes even if file read failed

[zampierilucas]

When using the -F, file, option, if the file read failed for any reason, the operation continues, ignoring the file comment.
In the example below, the user will unapprove the MR without submitting the message that he wanted.

lzampier@trusty:~$ lab mr unapprove origin -F a_file_that_doest_exists
2021/07/02 10:53:46 ERROR: note_common.go:25: open a_file_that_doest_existis: no such file or directory

it is possible that the same problem may occur in other parts of Lab that provides the -F option, a global solution for that would be:

diff --git a/cmd/note_common.go b/cmd/note_common.go
index 789c3a9..08da617 100644
--- a/cmd/note_common.go
+++ b/cmd/note_common.go
@@ -4,6 +4,7 @@ import (
        "bytes"
        "fmt"
        "io/ioutil"
+       "os"
        "runtime"
        "strconv"
        "strings"
@@ -23,6 +24,7 @@ func createNote(rn string, isMR bool, idNum int, msgs []string, filename string,
                content, err := ioutil.ReadFile(filename)
                if err != nil {
                        log.Fatal(err)
+                       os.Exit(1)
                }
                body = string(content)
        } else {

Should Lab take care of that? if so let me know so I can do further testing and can submit a PR :)

[zampierilucas]

@bmeneguele @prarit any input on this one? :)

[prarit]

Huh, I thought the log.Fatal(err) would have stopped lab. But yes, an os.Exit(1) there is fine.

[zampierilucas]

@prarit on a quick search, I found that log.Fatal() should call os.Exit(1).
I will investigate further this issue

[bmeneg]

Oh, yes. The log.Fatal() calls the os.Exit() it should not help to solve the problem.
The issue here might be the way log.Fatal() works, it doesn't close file descriptors nor running deferred code.
#729 might be related.

[bmeneg]

Oh, yes. The log.Fatal() calls the os.Exit() it should not help to solve the problem.

Just to clarify: even though we have a custom log layer, our log.Fatal() still calls the default golang/logging log.Fatal(), so the behavior is kept.

lab/internal/logger/logger.go

Lines 117 to 133 in 833faeb

	// Fatal prints the values and exit the program with os.Exit()
	func (l *Logger) Fatal(values ...interface{}) {
	values = append([]interface{}{addFileLinePrefix(" ")}, values...)
	l.errorLogger.Fatal(values...)
	}
	// Fatalf prints formated strings and exit the program with os.Exit()
	func (l *Logger) Fatalf(format string, values ...interface{}) {
	values = append([]interface{}{addFileLinePrefix("")}, values...)
	l.errorLogger.Fatalf("%s "+format, values...)
	}
	// Fatalln prints the values in a new line and exit the program with os.Exit()
	func (l *Logger) Fatalln(values ...interface{}) {
	values = append([]interface{}{addFileLinePrefix(" ")}, values...)
	l.errorLogger.Fatalln(values...)
	}

[zampierilucas]

Well, I'm sorry, the issue is far simpler than I initially thought, lab.MRApprove is called before CreateNote 🤦, so log.Fatal() seems to be working as expected, at least here.

While a simple fix would be to just move the function up with:

diff --git a/cmd/mr_approve.go b/cmd/mr_approve.go
index cd6a932..8e82775 100644
--- a/cmd/mr_approve.go
+++ b/cmd/mr_approve.go
@@ -48,6 +48,15 @@ var mrApproveCmd = &cobra.Command{
                        log.Fatal(err)
                }

+               if comment || len(msgs) > 0 || filename != "" {
+                       linebreak, err := cmd.Flags().GetBool("force-linebreak")
+                       if err != nil {
+                               log.Fatal(err)
+                       }
+
+                       createNote(rn, true, int(id), msgs, filename, linebreak, "")
+               }
+
                err = lab.MRApprove(p.ID, int(id))
                if err != nil {
                        if err == lab.ErrStatusForbidden {
@@ -59,15 +68,6 @@ var mrApproveCmd = &cobra.Command{
                        }
                }

-               if comment || len(msgs) > 0 || filename != "" {
-                       linebreak, err := cmd.Flags().GetBool("force-linebreak")
-                       if err != nil {
-                               log.Fatal(err)
-                       }
-
-                       createNote(rn, true, int(id), msgs, filename, linebreak, "")
-               }
-
                fmt.Printf("Merge Request !%d approved\n", id)
        },
 }

The problem with that fix is that it does not prevent a comment from being added when the un/approve does fail, so we are solving a problem and creating a new one.

I think the optimal solution would be to unify the comment and approve action in one API call, unfortunately, GitLab does not provide an API explicit for that.
I've found a hack by using GitLab quick-actions, If we add the text /approve or /unapprove(this has a caveat) to a note, we can make the un/approve + comment action in one call, but I'm not particularly satisfied with that, what do you guys think?