refactor: improve maintainability across pkg/ and cmd/ by bketelsen · Pull Request #54 · frostyard/nbc

bketelsen · 2026-02-25T18:08:52Z

Summary

Comprehensive refactoring to improve code maintainability, consistency, and modern Go patterns across the codebase. This is a 7-phase refactoring that touches 48 files with a net reduction of ~130 lines despite adding new abstractions.

Changes by phase:

Reporter interface — Replace concrete *ProgressReporter with a Reporter interface (TextReporter, JSONReporter, NoopReporter), enabling cleaner testing and output mode switching
Delete BootcInstaller — Remove the deprecated 862-LOC BootcInstaller type and its tests, relocating standalone utility functions to focused files (pkg/system.go, pkg/tools.go)
Type consolidation — Consolidate duplicate RebootPendingInfo to a single definition in pkg/types/
Context propagation + shared steps — Add context.Context to all exported I/O functions; extract SetupTargetSystem and ExtractAndVerifyContainer into pkg/steps.go to DRY up install/update flows; add Workflow type for composable step orchestration
Reporter everywhere — Replace all raw fmt.Print calls in pkg/ with Reporter interface methods, threading the reporter through functions that previously wrote directly to stdout/stderr
CLI flag structs — Encapsulate module-level flag variables into typed structs per command (installFlags, updateFlags, etc.)
Cleanup — Update CLAUDE.md to document new patterns and components

Key architectural improvements:

pkg/steps.go enforces install/update parity — shared operations are defined once
All pkg/ output goes through Reporter, making JSON mode consistent and tests quiet
context.Context flows through all I/O paths for proper cancellation
CLI flags are grouped by command instead of scattered as module-level vars

Test plan

make build passes
make test-unit passes (all tests except pre-existing TestCLI_VersionOutput golden fixture mismatch from dirty git state)
make lint passes with 0 issues
make test-integration (requires root)
Manual smoke test of nbc install and nbc update flows

🤖 Generated with Claude Code

Seven-phase plan covering: Reporter interface extraction, BootcInstaller removal, type consolidation, workflow pattern for install/update parity, raw output elimination, CLI flag structs, and cleanup pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

7-phase plan with bite-sized tasks, TDD steps, exact file paths, code snippets, and verification checklist. Covers Reporter interface, BootcInstaller deletion, type consolidation, workflow pattern, raw output elimination, CLI flag structs, and cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace all *ProgressReporter parameters with the Reporter interface across 30 files. This includes: - Function signatures: 35+ functions changed from progress *ProgressReporter to progress Reporter - Struct fields: Installer.progress, SystemUpdater.Progress, ContainerExtractor.Progress, BootloaderInstaller.Progress, BootcInstaller.Progress all changed to Reporter - Constructors: NewTextReporter/NewJSONReporter/NoopReporter used instead of NewProgressReporter - Step() calls: Updated from 2-arg Step(step, name) to 3-arg Step(step, total, name) to match new interface - Test files: All NewProgressReporter(false, 1) replaced with NoopReporter{} - cmd/ callers: Conditional Reporter creation pattern used The old ProgressReporter type in pkg/progress.go is intentionally kept for removal in the next task. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Delete InstallCallbacks, SetCallbacks, callOn* methods, callbackProgressAdapter, CreateCLICallbacks, and asLegacyProgress from pkg/install.go. Replace all callback invocations with direct Reporter method calls (Step, Message, MessagePlain, Warning, Error). Delete pkg/progress.go entirely -- the old ProgressReporter struct, type aliases, and re-exported constants are all superseded by pkg/reporter.go. Remove callback setup from cmd/install.go and cmd/interactive_install.go since NewInstaller now creates the appropriate Reporter internally based on cfg.JSONOutput. Delete tests for removed functionality: TestInstaller_SetCallbacks, TestInstaller_CallOnError, TestCreateCLICallbacks, and TestCallbackProgressAdapter. Update integration tests to remove SetCallbacks calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Move CheckRequiredTools to pkg/tools.go, PullImage to pkg/container.go, and SetRootPasswordInTarget to pkg/system.go in preparation for removing the deprecated BootcInstaller type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ypes Remove duplicate RebootPendingInfo from pkg/config.go, use types.RebootPendingInfo everywhere. Single source of truth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add ctx context.Context as the first parameter to all exported functions in pkg/ that do I/O or run external commands but previously lacked it. Convert exec.Command to exec.CommandContext where applicable to enable cancellation for long-running external commands. Updated functions: Download, Remove, Clear (ImageCache), InstallTmpfilesConfig, WriteSystemConfig, WriteSystemConfigToVar, UpdateSystemConfigImageRef, GetCurrentBootDeviceInfo, InstallDracutEtcOverlay, VerifyDracutEtcOverlay, SetupEtcOverlay, SetupEtcPersistence, InstallEtcMountUnit, PopulateEtcLower, SavePristineEtc, MergeEtcFromActive, EnsureCriticalFilesInOverlay, SetupSystemDirectories, PrepareMachineID, SetupLoopbackInstall, Cleanup (LoopbackDevice), SetRootPasswordInTarget. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Remove pkg/bootc.go and pkg/bootc_test.go entirely. The newer Installer API (NewInstaller) fully replaces it. Standalone functions were preserved in tools.go, container.go, system.go in prior commit. Update Makefile test-install target and copilot-instructions.md. Migrate readConfigFromFile helper to update_test.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Workflow runs a sequence of named StepFuncs with context cancellation and Reporter-based progress reporting. WorkflowState carries shared mutable state between steps. Both Install and Update flows will compose from shared steps using this abstraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…erifyContainer Deduplicate common install/update operations by extracting two shared helper functions in pkg/steps.go: - SetupTargetSystem: dracut module install, system directories, machine-id, etc.lower overlay, tmpfiles.d config - ExtractAndVerifyContainer: container extraction with verification Both install.go and update.go now call these shared functions instead of inline duplicated logic, removing ~115 lines of duplication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Migrate all business logic output in pkg/ to use the Reporter interface instead of raw fmt.Print/Fprintf calls: - partition.go: warnings now use progress.Warning - dracut.go: output uses progress.Warning/MessagePlain - bootloader.go: add Reporter param to EFI/BOOT rename helpers - luks.go: add Reporter param to EnrollTPM2 - device_detect.go: add Reporter param to GetCurrentBootDevice - update.go: add Reporter param to GetInactiveRootPartition - lint.go: add Reporter field to Linter, remove stdout redirect hack - cache.go: add Reporter field to ImageCache, remove nil fallbacks Remaining fmt.Print calls are intentional: interactive prompts (LUKS passphrase, confirmation dialog), terminal spinner UI, Reporter internals, and test infrastructure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace module-level flag variables with typed flag structs for each command (installFlags, updateFlags, downloadFlags, lintFlags, etc.), improving code organization and making flag ownership explicit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update key components to document Reporter interface, shared steps, and Workflow type. Update console output conventions, install/update parity section, and data flow to reflect context propagation and typed flag structs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This is a comprehensive 7-phase refactoring that improves code maintainability, consistency, and modern Go patterns across the nbc codebase. The refactoring touches 48 files with a net reduction of ~130 lines while adding significant new abstractions. The PR replaces the concrete *ProgressReporter with a Reporter interface, removes 862 LOC of deprecated BootcInstaller code, consolidates duplicate types, adds context propagation throughout, and introduces shared step functions that enforce install/update parity by design.

Changes:

Introduced Reporter interface with TextReporter, JSONReporter, and NoopReporter implementations to decouple output handling
Removed deprecated BootcInstaller and relocated utility functions to focused files (tools.go, system.go)
Added context.Context to all exported I/O functions and created shared SetupTargetSystem and ExtractAndVerifyContainer functions in pkg/steps.go to DRY up install/update flows
Encapsulated CLI flag variables into typed structs per command (eliminating module-level vars)

Reviewed changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/reporter.go	New Reporter interface with TextReporter, JSONReporter, NoopReporter implementations
pkg/reporter_test.go	Comprehensive tests for all Reporter implementations
pkg/workflow.go	New Workflow type for composable step-based orchestration
pkg/workflow_test.go	Tests for Workflow including error handling and cancellation
pkg/steps.go	Shared SetupTargetSystem and ExtractAndVerifyContainer functions enforce install/update parity
pkg/system.go, pkg/system_test.go	Relocated SetRootPasswordInTarget from deleted bootc.go
pkg/tools.go	Relocated CheckRequiredTools from deleted bootc.go
pkg/bootc.go, pkg/bootc_test.go	Deleted 862 LOC deprecated BootcInstaller code
pkg/progress.go	Deleted - replaced by reporter.go
pkg/install.go, pkg/update.go	Updated to use Reporter interface and shared steps, removed callback adapter code
pkg/partition.go, pkg/luks.go, pkg/disk.go, etc.	Changed from *ProgressReporter to Reporter parameter
pkg/etc_persistence.go, pkg/dracut.go, pkg/container.go, etc.	Added context.Context parameters and Reporter interface
pkg/config.go	Removed duplicate RebootPendingInfo, added context parameters
cmd/install.go, cmd/update.go, cmd/cache.go, etc.	Encapsulated flags into typed structs, removed module-level vars
cmd/interactive_install.go	Removed InstallCallbacks usage
docs/plans/2026-02-25-refactoring-design.md	Comprehensive refactoring design documentation
CLAUDE.md, .github/copilot-instructions.md	Updated to reflect new patterns and shared steps
Makefile	Updated test regex from TestBootcInstaller to TestInstaller

Copilot · 2026-02-25T18:32:43Z

pkg/update_test.go

+func readConfigFromFile(path string) (*SystemConfig, error) {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return nil, err
+	}
+
+	var config SystemConfig
+	if err := json.Unmarshal(data, &config); err != nil {
+		return nil, err
+	}
+
+	return &config, nil
+}


The readConfigFromFile helper function in update_test.go is only used by tests in that file. Consider moving it to a shared test helper file or internal test utilities package to make it more discoverable and reusable across test files. This would follow the pattern of other test utilities in pkg/testutil/.

Copilot · 2026-02-25T18:32:43Z

pkg/device_detect.go


 // GetCurrentBootDevice determines the disk device that the system booted from
-func GetCurrentBootDevice() (string, error) {
+func GetCurrentBootDevice(progress Reporter) (string, error) {


GetCurrentBootDevice is missing a context.Context parameter while GetCurrentBootDeviceInfo has one. For consistency and cancellation support, GetCurrentBootDevice should also accept ctx context.Context as its first parameter. This is especially important since it performs I/O operations that could benefit from cancellation.

Copilot · 2026-02-25T18:32:44Z

Makefile


 _test-install: ## Internal target for install tests
-	@go test -v ./pkg/... -run "^(TestBootcInstaller)" -timeout 20m
+	@go test -v ./pkg/... -run "^(TestInstaller)" -timeout 20m


The test regex pattern ^(TestInstaller) will match any test function starting with "TestInstaller", including "TestInstaller_Install", "TestInstaller_WithEncryption", etc. However, this pattern is less explicit than the original ^(TestBootcInstaller). Consider using ^TestInstaller (without parentheses) or ^TestIntegration_Installer to be more precise about which tests to run.

Suggested change

@go test -v ./pkg/... -run "^(TestInstaller)" -timeout 20m

@go test -v ./pkg/... -run "^TestIntegration_Installer" -timeout 20m

cmd/install.go

bketelsen and others added 14 commits February 25, 2026 09:58

feat: add Reporter interface with Text, JSON, and Noop implementations

d445682

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor: consolidate RebootPendingInfo to single definition in pkg/t…

0e53412

…ypes Remove duplicate RebootPendingInfo from pkg/config.go, use types.RebootPendingInfo everywhere. Single source of truth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

bketelsen requested a review from Copilot February 25, 2026 18:28

Copilot started reviewing on behalf of bketelsen February 25, 2026 18:29 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

bketelsen merged commit 9776bc5 into main Feb 25, 2026
12 checks passed

bketelsen deleted the emdash/refactoring-things-5u4 branch February 25, 2026 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: improve maintainability across pkg/ and cmd/#54

refactor: improve maintainability across pkg/ and cmd/#54
bketelsen merged 14 commits intomainfrom
emdash/refactoring-things-5u4

bketelsen commented Feb 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	@go test -v ./pkg/... -run "^(TestInstaller)" -timeout 20m
	@go test -v ./pkg/... -run "^TestIntegration_Installer" -timeout 20m

Conversation

bketelsen commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes by phase:

Key architectural improvements:

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bketelsen commented Feb 25, 2026 •

edited

Loading