feat: add initial operations - #665
Conversation
d571ae1 to
b697639
Compare
howieandersen
left a comment
There was a problem hiding this comment.
Jeg har gått gjennom hele diffen og verifisert de kritiske delene mot CommandLine-rammeverket og BCL-oppførsel. Opprydningen fra TS-skript til repoctl ser fin ut, og CI-flyten med matrix.path fungerer som den skal. Jeg fant likevel to feil i sti-håndteringen som bør fikses først:
1. Stier med trailing separator matcher aldri i IsDescendantOf
FileSystemHelpers.cs:12 sammenligner current.FullName == ancestor.FullName som rene strenger. Path.GetFullPath bevarer trailing separator fra input, og DirectoryInfo.FullName beholder den, mens .Parent.FullName aldri har trailing separator. Verifisert på .NET 10: Path.GetFullPath("src/", root) gir <root>/src/, mens parent-vandringen fra en undermappe gir <root>/src. Strengene blir aldri like.
Konsekvens: repoctl verticals list --dir src/ gir tom liste uten feilmelding (hjelpeteksten viser ./ som default, som har samme problem), og repoctl vertical test src/Altinn.Urn/ feiler med "No vertical found" fordi tab-fullføring legger på skråstrek. Gjelder begge oppslagene i AltinnRepositoryBinderResolver.cs:117 og :163. Path.TrimEndingDirectorySeparator på begge sider av sammenligningen løser det.
2. VerticalResolver returnerer null og krasjer med NullReferenceException
AltinnRepositoryBinderResolver.cs:170 og :182 skriver feilmelding, setter ReturnCode = 1 og returnerer null. Rammeverket sjekker ikke ReturnCode mellom parameteroppløsning og handler-kall: Handle.Invoke i CommandHandlerDelegateFactory.cs:604 awaiter argument-taskene og kaller handleren uansett. Null treffer da vertical.Projects i TestService.cs:12 og PackService.cs:13, så brukeren får den pene feilmeldingen etterfulgt av en NRE. Kast heller en exception her (samme mønster som repo.EnsureSuccess()), eller legg inn kortslutning på ReturnCode i rammeverket.
Sammen betyr 1 og 2 at repoctl vertical test src/Altinn.Urn/ i dag gir feilmelding pluss stack trace.
Mindre ting, ikke blokkerende:
- AltinnVerticalConfiguration.cs:140:
TryValidateChild("/terraform", ...)uten null-guard gjør at"infra": {}avvises med Required (InputModelValidator legger på StdValidationErrors.Required for null input). Det gamle zod-schemaet hadde terraform som optional, og både nullableTerraform-property ogHasTerraform-sjekken i FindVerticalsCommand tyder på at den skal være valgfri. Hvis required er meningen kan propertyen like gjerne være non-nullable. - GetPathFiltersCommand.cs:38:
filters[dep.Id.ToString()]kaster KeyNotFoundException når en dependency ligger utenfor det filtrerte settet, f.eks.repoctl ci get-path-filters --dir src/Altinn.Authorization.ServiceDefaults(alle dependencies ligger utenfor katalogen). TryGetValue, eller bygg filters for alle verticals og filtrer til slutt. - De gamle skriptene kjørte både nuget push og asset-upload med
retry(5, ...). NuGetService.cs:41 og GitHubService.cs:73 gjør ett forsøk. Asset-upload mot GitHub er notorisk flaky, og--skip-duplicategjør push trygg å kjøre på nytt, så retry bør inn igjen før neste release. - GitHubService.cs:42: Octokit kaster NotFoundException på 404, så
release is null-sjekken er død kode. - Directory.Packages.props mistet newline på slutten av filen.
No description provided.