Merge pull request 'Accept a bare integer as seconds for watchpr --interval' (#11) from benvin/watchpr-interval-units into main

Reviewed-on: #11

README.md

@@ -70,6 +70,9 @@ watchpr unkin/argocd-apps#42
 # Multiple PRs, custom interval; refs accept #N or :N
 watchpr --interval 30s unkin/argocd-apps#42 unkin/terraform-vault:98
 
+# --interval takes a duration (30s, 2m, 1h30m) or a bare number of seconds
+watchpr --interval 30 unkin/argocd-apps#42
+
 # One-shot: print current state and exit 0 (great for scripts)
 watchpr --once unkin/argocd-apps#42
 watchpr --once --json unkin/argocd-apps#42

cmd/watchpr/main.go

@@ -7,6 +7,7 @@
 //	watchpr owner/repo#12 owner/repo:15
 //	watchpr --once --json owner/repo#12
 //	watchpr --interval 30s owner/repo#12
+//	watchpr --interval 30 owner/repo#12
 package main
 
 import (
@@ -34,7 +35,7 @@ func main() {
 // tests can invoke Execute and assert the exit behaviour without spawning a
 // process.
 func newRootCmd() *cobra.Command {
-	var interval time.Duration
+	var intervalFlag string
 	var once, jsonMode bool
 
 	root := &cobra.Command{
@@ -50,6 +51,10 @@ func newRootCmd() *cobra.Command {
 			if len(args) == 0 {
 				return fmt.Errorf("no PR references given (e.g. owner/repo#12)")
 			}
+			interval, err := agent.ParseDurationFlag("interval", intervalFlag)
+			if err != nil {
+				return err
+			}
 			refs := make([]agent.PRRef, 0, len(args))
 			for _, a := range args {
 				ref, err := agent.ParsePRRef(a)
@@ -68,7 +73,7 @@ func newRootCmd() *cobra.Command {
 	root.SetVersionTemplate("{{.Version}}\n")
 
 	f := root.Flags()
-	f.DurationVar(&interval, "interval", 60*time.Second, "Polling interval")
+	f.StringVar(&intervalFlag, "interval", "60s", "Polling interval: a duration (30s, 2m, 1h30m) or a bare number of seconds")
 	f.BoolVar(&once, "once", false, "Check once, print current state, and exit")
 	f.BoolVar(&jsonMode, "json", false, "Emit JSON")
 

cmd/watchpr/main_test.go

@@ -4,6 +4,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 )
 
@@ -70,3 +71,37 @@ func TestOnceRunsAnonymouslyWhenNoTokenIsAvailable(t *testing.T) {
 		t.Errorf("sent %d Authorization headers, want none", authHeaders)
 	}
 }
+
+// A bare integer interval means seconds and must survive flag parsing: the
+// command should fail on the missing PR reference, not on the flag value.
+func TestExecuteBareIntervalIsSeconds(t *testing.T) {
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"--interval", "15"})
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("Execute() = nil, want the no-references error")
+	}
+	if !strings.Contains(err.Error(), "no PR references given") {
+		t.Fatalf("Execute() error = %v, want the no-references error", err)
+	}
+}
+
+// An unparseable interval is rejected before any Vault/Gitea call, with an
+// error naming the flag and showing valid forms.
+func TestExecuteBadIntervalErrors(t *testing.T) {
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"--interval", "soon", "unkin/repo#1"})
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("Execute() = nil, want error for an unparseable --interval")
+	}
+	for _, want := range []string{"--interval", "30s"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("Execute() error %q does not mention %q", err, want)
+		}
+	}
+}

internal/agent/parse.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"time"
 )
 
 // PRRef identifies a single pull request by repository and number.
@@ -48,6 +49,25 @@ func ParsePRRef(s string) (PRRef, error) {
 	return PRRef{Owner: owner, Repo: repo, Number: n}, nil
 }
 
+// ParseDurationFlag parses a duration flag value, accepting either a Go
+// duration string ("30s", "1h30m") or a bare integer read as seconds ("15").
+// flag names the flag so the error says which value was rejected.
+func ParseDurationFlag(flag, value string) (time.Duration, error) {
+	s := strings.TrimSpace(value)
+	d, err := time.ParseDuration(s)
+	if err != nil {
+		n, nerr := strconv.Atoi(s)
+		if nerr != nil {
+			return 0, fmt.Errorf("invalid --%s value %q: want a duration such as 30s, 2m or 1h30m, or a bare number of seconds such as 15", flag, value)
+		}
+		d = time.Duration(n) * time.Second
+	}
+	if d <= 0 {
+		return 0, fmt.Errorf("invalid --%s value %q: must be greater than zero", flag, value)
+	}
+	return d, nil
+}
+
 // ParseRepo validates and splits an "owner/repo" string.
 func ParseRepo(s string) (owner, repo string, err error) {
 	s = strings.TrimSpace(s)

internal/agent/parse_test.go

@@ -1,6 +1,10 @@
 package agent
 
-import "testing"
+import (
+	"strings"
+	"testing"
+	"time"
+)
 
 func TestParsePRRef(t *testing.T) {
 	tests := []struct {
@@ -77,3 +81,54 @@ func TestParseRepo(t *testing.T) {
 		}
 	}
 }
+
+func TestParseDurationFlag(t *testing.T) {
+	tests := []struct {
+		in      string
+		want    time.Duration
+		wantErr bool
+	}{
+		{"15", 15 * time.Second, false},
+		{"15s", 15 * time.Second, false},
+		{"2m", 2 * time.Minute, false},
+		{"1h30m", 90 * time.Minute, false},
+		{"500ms", 500 * time.Millisecond, false},
+		{" 45 ", 45 * time.Second, false},
+		{"0", 0, true},
+		{"0s", 0, true},
+		{"-5", 0, true},
+		{"-5s", 0, true},
+		{"15x", 0, true},
+		{"", 0, true},
+	}
+	for _, tt := range tests {
+		got, err := ParseDurationFlag("interval", tt.in)
+		if tt.wantErr {
+			if err == nil {
+				t.Errorf("ParseDurationFlag(%q): expected error, got %v", tt.in, got)
+			}
+			continue
+		}
+		if err != nil {
+			t.Errorf("ParseDurationFlag(%q): unexpected error: %v", tt.in, err)
+			continue
+		}
+		if got != tt.want {
+			t.Errorf("ParseDurationFlag(%q) = %v, want %v", tt.in, got, tt.want)
+		}
+	}
+}
+
+// The error must name the flag and show valid forms instead of surfacing
+// time.ParseDuration's "missing unit" wording.
+func TestParseDurationFlagErrorMessage(t *testing.T) {
+	_, err := ParseDurationFlag("interval", "soon")
+	if err == nil {
+		t.Fatal("ParseDurationFlag(\"soon\"): expected error")
+	}
+	for _, want := range []string{"--interval", `"soon"`, "30s", "seconds"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not mention %q", err, want)
+		}
+	}
+}