refactor(config): return errors instead of exiting the process#75
Merged
Conversation
config/ called log.Fatal (= os.Exit(1)) on every failure, so it could only ever be used from a CLI: any library or server consumer would be killed mid-request by a config typo. Turn the 17 exit sites into errors and thread them out through GetAssignmentConfig/GetCourseConfig; cmd/ is now the only layer that terminates the process. The goldens from the previous commit pass unchanged, which is the point: this is a signature change, not a behaviour change. Cross-checked independently by diffing `glabs show` output for all 13 assignments in mpd/vss against a binary built before the refactor — identical. (That comparison needs sorted output: config.Show() renders deferredBranches straight from a Go map, so its ordering already varies between runs today. Cosmetic, pre-existing, left alone here.) Error paths are now testable at all, and config/errors_test.go covers all 15 of them against a new testdata/errors/edge.yaml. It lives outside testdata/courses/ so the golden test does not try to resolve fixtures that are meant to fail. Two bare `log.Fatal()` calls in seeder.go turn out to have been worse than they looked. zerolog only exits in Msg()/Send(), so a Fatal() with no message neither logs nor exits: execution fell through to entities[0] on a nil slice and panicked with index out of range. Both are now errors, with the missing len(entities) and PrivateKey == nil guards added. The viper.Set() write-back in the inheritance read path stays for now. Removing it means threading access to the merged map through ~63 readers — exactly the work the typed source schema does next, which would then delete that intermediate again. It disappears for free there, when Resolve() becomes pure by construction. Documented in place until then. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Schritt 1 des glabs-web-Plans. Ersetzt #74, das GitHub automatisch schloss, als der Base-Branch beim Merge von #73 gelöscht wurde. Inhalt unverändert, auf
mainrebast.Warum
config/rief bei jedem Fehlerlog.Fatal(=os.Exit(1)). Damit ist das Paket ausschließlich aus einem CLI heraus benutzbar: ein Bibliotheks- oder Server-Konsument würde von einem Config-Tippfehler mitten im Request abgeschossen. Die 17 Exit-Stellen werden zuerror, durchgereicht überGetAssignmentConfig/GetCourseConfig.cmd/ist danach die einzige Schicht, die den Prozess beendet.Verhaltenserhaltend — und zwar nachgewiesen
glabs showfür alle 13 Assignments aus mpd/vss gegen ein Binary vom Stand vor dem Umbau gediffed → identisch.Show()rendert anders als der Golden-View, insofern prüft das eine andere Seite.Fußnote dazu: der Vergleich braucht sortierten Output.
config.Show()gibtdeferredBranchesdirekt aus einer Go-Map aus, die Reihenfolge variiert also schon heute zwischen zwei Läufen desselben Binaries. Kosmetisch, vorbestehend, hier bewusst nicht angefasst (Show()wird in Schritt 5 ohnehin auf den Reporter umgebaut). Die Goldens sind davon nicht betroffen, weilencoding/jsonMap-Keys sortiert.Der eigentliche Gewinn: Fehlerpfade sind erstmals testbar
config/errors_test.godeckt alle 15 ab — unbekannter Kurs/Assignment, abstraktes Assignment direkt benutzt,extends-Zyklus (über zwei Assignments und auf sich selbst),extendskein String / leer / Parent fehlt, startercode ohne url, seeder ohne cmd, kaputter signKey, Legacy-users-Key, ungültigeswhenCommitAdded. Jeder einzelne davon hätte vorher den Testprozess gekillt.Das neue
testdata/errors/edge.yamlliegt bewusst außerhalb vontestdata/courses/, damit der Golden-Test nicht versucht, Fixtures aufzulösen, die scheitern sollen.Fund: die zwei nachrichtenlosen
log.Fatal()in seeder.go waren schlimmer als gedachtIch hatte sie beim Planen als „exit mit leerer Logzeile" notiert. Falsch: zerolog löst den Exit erst in
Msg()/Send()aus. Einlog.Fatal()ohne Message loggt also nichts und beendet nichts — die Ausführung lief durch aufentities[0]eines leeren Slices und panickte mit index out of range. Empirisch verifiziert.Beide sind jetzt echte Fehler, plus die fehlenden
len(entities)- undPrivateKey == nil-Guards. Latent (kein echtes YAML nutztseeder.signKey), aber der Fehlerpfad ist jetzt einer.Bewusste Abweichung vom Plan: das
viper.SetbleibtDer Plan sah vor, den Write-Back aus dem Lesepfad (
config/inheritance.go:54) hier zu entfernen. Das hieße, Zugriff auf die gemergte Map durch ~63 Reader zu fädeln — exakt die Arbeit, die Schritt 2 macht und die Schritt 2 dann sofort wieder wegwirft, weilResolve()dort per Konstruktion rein wird. Der Zwischenschritt wäre zu 100% Wegwerfarbeit.Es verschwindet in Schritt 2 gratis. Bis dahin ist es an Ort und Stelle dokumentiert, inklusive der Konsequenz (nicht nebenläufigkeitssicher, Aufrufer müssen ein Assignment nach dem anderen auflösen).
Damit stimmt auch die Plan-Behauptung „config/-Tests brauchen kein
viper.Reset()mehr" für diesen Schritt noch nicht — die gilt erst nach Schritt 2, wennconfig/den viper-Global gar nicht mehr liest.Verifikation
go test ./...grün;gofmt,go vet,golangci-lintsauber-updategrün → kein Verhaltensdriftglabs showOutput vor/nach identisch (13/13)🤖 Generated with Claude Code