From 26c27023d7a794a9bf09fa4cf1da6f8e242cf157 Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 29 Jan 2026 20:56:48 -0500 Subject: [PATCH 1/5] Fix logs command hanging due to output wrapper buffering The velero->oadp output wrapper was causing logs commands to hang because it buffered all output before processing. For streaming commands like logs, this created a deadlock since the command never fully completes. Solution: Skip wrapping logs commands entirely to allow real-time streaming without buffering. This also prevents log content from being modified, which could hide important velero-related information in actual logs. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/root.go | 57 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 4fd2635..907217b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -208,8 +208,11 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { // Replace in multiple command fields using context-aware replacement cmd.Example = replaceVeleroCommandWithOADP(cmd.Example) + // Skip wrapping logs commands to allow real-time streaming without buffering + isLogsCommand := cmd.Use == "logs" || strings.HasPrefix(cmd.Use, "logs ") + // Wrap the Run function to replace velero in output - if cmd.Run != nil { + if cmd.Run != nil && !isLogsCommand { originalRun := cmd.Run cmd.Run = func(c *cobra.Command, args []string) { // Capture stdout temporarily @@ -217,26 +220,37 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { r, w, _ := os.Pipe() os.Stdout = w + // Start goroutine to read from pipe and write to real stdout + // This prevents deadlock with streaming commands (like logs) + done := make(chan error, 1) + go func() { + var buf strings.Builder + _, err := io.Copy(&buf, r) + if err != nil { + done <- err + return + } + output := replaceVeleroCommandWithOADP(buf.String()) + fmt.Fprint(oldStdout, output) + done <- nil + }() + // Run the original command originalRun(c, args) - // Restore stdout + // Close writer to signal EOF to the reading goroutine w.Close() os.Stdout = oldStdout - // Read captured output and replace velero with oadp (context-aware) - var buf strings.Builder - _, err := io.Copy(&buf, r) - if err != nil { + // Wait for the goroutine to finish reading and writing + if err := <-done; err != nil { fmt.Fprintf(os.Stderr, "WARNING: Error copying output: %v\n", err) } - output := replaceVeleroCommandWithOADP(buf.String()) - fmt.Print(output) } } // Wrap the RunE function to replace velero in output - if cmd.RunE != nil { + if cmd.RunE != nil && !isLogsCommand { originalRunE := cmd.RunE cmd.RunE = func(c *cobra.Command, args []string) error { // Capture stdout temporarily @@ -244,21 +258,32 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { r, w, _ := os.Pipe() os.Stdout = w + // Start goroutine to read from pipe and write to real stdout + // This prevents deadlock with streaming commands (like logs) + done := make(chan error, 1) + go func() { + var buf strings.Builder + _, copyErr := io.Copy(&buf, r) + if copyErr != nil { + done <- copyErr + return + } + output := replaceVeleroCommandWithOADP(buf.String()) + fmt.Fprint(oldStdout, output) + done <- nil + }() + // Run the original command err := originalRunE(c, args) - // Restore stdout + // Close writer to signal EOF to the reading goroutine w.Close() os.Stdout = oldStdout - // Read captured output and replace velero with oadp (context-aware) - var buf strings.Builder - _, copyErr := io.Copy(&buf, r) - if copyErr != nil { + // Wait for the goroutine to finish reading and writing + if copyErr := <-done; copyErr != nil { fmt.Fprintf(os.Stderr, "WARNING: Error copying output: %v\n", copyErr) } - output := replaceVeleroCommandWithOADP(buf.String()) - fmt.Print(output) return err } From f092cd06cff9faea6fe7aa598a2c399a3603e48f Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 29 Jan 2026 22:01:15 -0500 Subject: [PATCH 2/5] Simplify output wrapper and add regression test for logs exclusion Removed goroutine complexity from the velero->oadp output wrapper since logs commands are now excluded. The remaining wrapped commands are all non-streaming and complete normally, so the simpler synchronous approach is sufficient. Added comprehensive test to ensure logs commands are never wrapped. The test verifies both Run and RunE functions are not wrapped for logs commands, and that wrapped commands still get output replacement while logs commands stream unmodified output. This prevents regressions if the exclusion logic is accidentally removed. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/root.go | 50 +++++------------ cmd/root_test.go | 140 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 36 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 907217b..f233354 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -220,32 +220,21 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { r, w, _ := os.Pipe() os.Stdout = w - // Start goroutine to read from pipe and write to real stdout - // This prevents deadlock with streaming commands (like logs) - done := make(chan error, 1) - go func() { - var buf strings.Builder - _, err := io.Copy(&buf, r) - if err != nil { - done <- err - return - } - output := replaceVeleroCommandWithOADP(buf.String()) - fmt.Fprint(oldStdout, output) - done <- nil - }() - // Run the original command originalRun(c, args) - // Close writer to signal EOF to the reading goroutine + // Restore stdout w.Close() os.Stdout = oldStdout - // Wait for the goroutine to finish reading and writing - if err := <-done; err != nil { + // Read captured output and replace velero with oadp (context-aware) + var buf strings.Builder + _, err := io.Copy(&buf, r) + if err != nil { fmt.Fprintf(os.Stderr, "WARNING: Error copying output: %v\n", err) } + output := replaceVeleroCommandWithOADP(buf.String()) + fmt.Print(output) } } @@ -258,32 +247,21 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { r, w, _ := os.Pipe() os.Stdout = w - // Start goroutine to read from pipe and write to real stdout - // This prevents deadlock with streaming commands (like logs) - done := make(chan error, 1) - go func() { - var buf strings.Builder - _, copyErr := io.Copy(&buf, r) - if copyErr != nil { - done <- copyErr - return - } - output := replaceVeleroCommandWithOADP(buf.String()) - fmt.Fprint(oldStdout, output) - done <- nil - }() - // Run the original command err := originalRunE(c, args) - // Close writer to signal EOF to the reading goroutine + // Restore stdout w.Close() os.Stdout = oldStdout - // Wait for the goroutine to finish reading and writing - if copyErr := <-done; copyErr != nil { + // Read captured output and replace velero with oadp (context-aware) + var buf strings.Builder + _, copyErr := io.Copy(&buf, r) + if copyErr != nil { fmt.Fprintf(os.Stderr, "WARNING: Error copying output: %v\n", copyErr) } + output := replaceVeleroCommandWithOADP(buf.String()) + fmt.Print(output) return err } diff --git a/cmd/root_test.go b/cmd/root_test.go index bc93d51..1a8c1e0 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -591,6 +591,146 @@ func TestApplyTimeoutToConfig(t *testing.T) { } } +// TestReplaceVeleroWithOADP_LogsCommandNotWrapped tests that logs commands are never wrapped +func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { + tests := []struct { + name string + use string + shouldWrap bool + }{ + { + name: "logs command", + use: "logs", + shouldWrap: false, + }, + { + name: "logs with args", + use: "logs NAME", + shouldWrap: false, + }, + { + name: "get command", + use: "get", + shouldWrap: true, + }, + { + name: "describe command", + use: "describe", + shouldWrap: true, + }, + { + name: "create command", + use: "create", + shouldWrap: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test with Run function + runCalled := false + cmd := &cobra.Command{ + Use: tt.use, + Run: func(c *cobra.Command, args []string) { + runCalled = true + fmt.Println("test output with velero backup create") + }, + } + + // Store reference to original Run function + originalRun := cmd.Run + + replaceVeleroWithOADP(cmd) + + // If logs command, Run should not be wrapped (same function pointer) + // If not logs, Run should be wrapped (different function pointer) + isWrapped := fmt.Sprintf("%p", originalRun) != fmt.Sprintf("%p", cmd.Run) + + if tt.shouldWrap && !isWrapped { + t.Errorf("Expected command %q to be wrapped, but it wasn't", tt.use) + } + if !tt.shouldWrap && isWrapped { + t.Errorf("Expected command %q NOT to be wrapped, but it was", tt.use) + } + + // Verify the command still executes + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + cmd.Run(cmd, []string{}) + w.Close() + os.Stdout = oldStdout + + var buf bytes.Buffer + io.Copy(&buf, r) + + if !runCalled { + t.Error("Original Run function was not called") + } + + output := buf.String() + if tt.shouldWrap { + // Wrapped commands should have output replaced + if strings.Contains(output, "velero backup create") { + t.Errorf("Wrapped command output should have 'velero' replaced, got: %s", output) + } + } else { + // Logs commands should NOT have output replaced + if !strings.Contains(output, "velero backup create") { + t.Errorf("Logs command output should NOT be modified, got: %s", output) + } + } + }) + } + + // Test with RunE function + t.Run("logs_command_runE_not_wrapped", func(t *testing.T) { + runECalled := false + cmd := &cobra.Command{ + Use: "logs", + RunE: func(c *cobra.Command, args []string) error { + runECalled = true + fmt.Println("test output with velero backup logs") + return nil + }, + } + + originalRunE := cmd.RunE + replaceVeleroWithOADP(cmd) + + // Logs command should not be wrapped + isWrapped := fmt.Sprintf("%p", originalRunE) != fmt.Sprintf("%p", cmd.RunE) + if isWrapped { + t.Error("Expected logs command RunE NOT to be wrapped, but it was") + } + + // Verify output is not modified + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + err := cmd.RunE(cmd, []string{}) + w.Close() + os.Stdout = oldStdout + + if err != nil { + t.Errorf("RunE returned error: %v", err) + } + + if !runECalled { + t.Error("Original RunE function was not called") + } + + var buf bytes.Buffer + io.Copy(&buf, r) + output := buf.String() + + // Logs command output should NOT be modified + if !strings.Contains(output, "velero backup logs") { + t.Errorf("Logs command output should NOT be modified, got: %s", output) + } + }) +} + // TestApplyTimeoutToConfig_DialerTimeout tests that the custom dialer respects the timeout func TestApplyTimeoutToConfig_DialerTimeout(t *testing.T) { // Set a very short timeout From 841e401b1dd039f19226abc29cff2ecbe415fe63 Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 29 Jan 2026 22:17:55 -0500 Subject: [PATCH 3/5] Fix linting issues Signed-off-by: Joseph --- cmd/root.go | 4 ++-- cmd/root_test.go | 16 ++++++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index f233354..56e046e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -224,7 +224,7 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { originalRun(c, args) // Restore stdout - w.Close() + _ = w.Close() os.Stdout = oldStdout // Read captured output and replace velero with oadp (context-aware) @@ -251,7 +251,7 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { err := originalRunE(c, args) // Restore stdout - w.Close() + _ = w.Close() os.Stdout = oldStdout // Read captured output and replace velero with oadp (context-aware) diff --git a/cmd/root_test.go b/cmd/root_test.go index 1a8c1e0..8843619 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -243,7 +243,7 @@ func TestReplaceVeleroWithOADP_RunFunctionWrapper(t *testing.T) { cmd.Run(cmd, []string{}) // Restore stdout - w.Close() + _ = w.Close() os.Stdout = oldStdout // Read captured output @@ -470,7 +470,7 @@ func TestReplaceVeleroWithOADP_RunOutputPreservesProperNouns(t *testing.T) { cmd.Run(cmd, []string{}) // Restore stdout - w.Close() + _ = w.Close() os.Stdout = oldStdout // Read captured output @@ -658,11 +658,13 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w cmd.Run(cmd, []string{}) - w.Close() + _ = w.Close() os.Stdout = oldStdout var buf bytes.Buffer - io.Copy(&buf, r) + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("Error copying output: %v", err) + } if !runCalled { t.Error("Original Run function was not called") @@ -709,7 +711,7 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w err := cmd.RunE(cmd, []string{}) - w.Close() + _ = w.Close() os.Stdout = oldStdout if err != nil { @@ -721,7 +723,9 @@ func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { } var buf bytes.Buffer - io.Copy(&buf, r) + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("Error copying output: %v", err) + } output := buf.String() // Logs command output should NOT be modified From b4ce39011eb23f0fe04c56964cea192130e1663c Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 29 Jan 2026 22:38:30 -0500 Subject: [PATCH 4/5] fmt Signed-off-by: Joseph --- cmd/root_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/root_test.go b/cmd/root_test.go index 8843619..cb0fb9b 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -594,9 +594,9 @@ func TestApplyTimeoutToConfig(t *testing.T) { // TestReplaceVeleroWithOADP_LogsCommandNotWrapped tests that logs commands are never wrapped func TestReplaceVeleroWithOADP_LogsCommandNotWrapped(t *testing.T) { tests := []struct { - name string - use string - shouldWrap bool + name string + use string + shouldWrap bool }{ { name: "logs command", From 9261c555f7bf85e379c80dfce75474f157df1fce Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 29 Jan 2026 23:06:14 -0500 Subject: [PATCH 5/5] Add lint and test targets following OADP operator pattern Added Makefile targets for linting and testing that follow the same approach as the OADP operator project: - lint: Runs golangci-lint v1.63.4 (auto-downloads to bin/ if needed) - test: Runs all tests (unit + integration) - test-unit: Runs unit tests only - test-integration: Runs integration tests only - golangci-lint: Helper target to install linter locally Tools are installed locally via 'go install' to bin/ directory instead of using containers, providing faster execution and matching the OADP operator development workflow. Cleanup: - Removed unused CONTAINER_TOOL variable - Removed unused get_binary_name function - Added bin/ to .gitignore to prevent committing downloaded tools - Updated clean target to remove bin/ directory Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- .gitignore | 3 +++ Makefile | 50 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index faf3a68..0bd2c3e 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,9 @@ kubectl-oadp-linux-* kubectl-oadp-darwin-* kubectl-oadp-windows-* +# Local development tools +bin/ + # Release artifacts *.tar.gz *.sha256 diff --git a/Makefile b/Makefile index 5fabffa..3f0b264 100644 --- a/Makefile +++ b/Makefile @@ -26,11 +26,6 @@ PLATFORM ?= GOOS = $(word 1,$(subst /, ,$(PLATFORM))) GOARCH = $(word 2,$(subst /, ,$(PLATFORM))) -# Helper function to get binary name with .exe for Windows -define get_binary_name -$(if $(findstring windows,$(1)),$(BINARY_NAME).exe,$(BINARY_NAME)) -endef - # Default target .PHONY: help help: ## Show this help message @@ -62,10 +57,11 @@ help: ## Show this help message @echo " make build PLATFORM=windows/amd64" @echo " make build PLATFORM=windows/arm64" @echo "" - @echo "Testing commands:" + @echo "Testing and linting commands:" @echo " make test # Run all tests (unit + integration)" @echo " make test-unit # Run unit tests only" @echo " make test-integration # Run integration tests only" + @echo " make lint # Run golangci-lint checks" @echo "" @echo "Release commands:" @echo " make release-build # Build binaries for all platforms" @@ -329,6 +325,41 @@ uninstall-all: ## Uninstall the kubectl plugin from all locations (user + system @make --no-print-directory uninstall @make --no-print-directory uninstall-system +# Local binary directory for development tools +LOCALBIN ?= $(shell pwd)/bin +$(LOCALBIN): + mkdir -p $(LOCALBIN) + +# Tool versions +GOLANGCI_LINT_VERSION ?= v1.63.4 + +# Tool binaries +GOLANGCI_LINT = $(LOCALBIN)/golangci-lint + +# go-install-tool will 'go install' any package $2 and install it to $1. +define go-install-tool +[ -f $(1) ] || { \ +set -e ;\ +TMP_DIR=$$(mktemp -d) ;\ +cd $$TMP_DIR ;\ +go mod init tmp ;\ +echo "Downloading $(2)" ;\ +GOBIN=$(LOCALBIN) go install $(2) ;\ +rm -rf $$TMP_DIR ;\ +} +endef + +# golangci-lint installation +.PHONY: golangci-lint +golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary +$(GOLANGCI_LINT): $(LOCALBIN) + @if [ -f $(GOLANGCI_LINT) ] && $(GOLANGCI_LINT) version 2>&1 | grep -q $(GOLANGCI_LINT_VERSION); then \ + echo "golangci-lint $(GOLANGCI_LINT_VERSION) is already installed"; \ + else \ + echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION)"; \ + $(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)); \ + fi + # Testing targets .PHONY: test test: ## Run all tests @@ -351,13 +382,18 @@ test-integration: ## Run integration tests only go test . -v @echo "✅ Integration tests completed!" +.PHONY: lint +lint: golangci-lint ## Run golangci-lint checks against all project's Go files + $(GOLANGCI_LINT) run ./... + # Cleanup targets .PHONY: clean -clean: ## Remove built binaries +clean: ## Remove built binaries and downloaded tools @echo "Cleaning up..." @rm -f $(BINARY_NAME) $(BINARY_NAME).exe $(BINARY_NAME)-linux-* $(BINARY_NAME)-darwin-* $(BINARY_NAME)-windows-* @rm -f *.tar.gz *.sha256 @rm -f oadp-*.yaml oadp-*.yaml.tmp + @rm -rf $(LOCALBIN) @echo "✅ Cleanup complete!" # Status and utility targets