feat: MinVer git-tag versioning + /version endpoint + container tags #23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/minver-versioning"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
MinVer git-tag-based semantic versioning
How it works
v0.1.0): MinVer produces0.1.0(clean)0.1.0-dev.N+shawhere N = commits since tag, sha = commit hashAssemblyInformationalVersion→ startup log,/versionendpoint, Docker container tagChanges
Directory.Packages.propsDirectory.Build.props(new)MinVerDefaultPreReleasePhase=devDoujinManager.Server.csprojProgram.csGET /versionendpointDockerfile.dockerignore.git/(MinVer needs it)docker-publish.ymlv*tags tooVersion endpoints
GET /version→{"version":"0.1.0-dev.5+abc1234"}(anonymous)DoujinManager Server v0.1.0-dev.5+abc1234git.kagaku.eu/teamai/doujin-manager:0.1.0-dev.5+abc1234What you need to do after merge
Create the baseline tag so MinVer has a starting point:
Future releases:
git tag v0.2.0 && git push --tags— the docker-publish pipeline will automatically build and tag the container.See ADR 0020 for the full decision record.
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 84.8%
DoujinManager.Infrastructure - 91.6%
pshot
DoujinManager.RestAdapter - 84.2%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 21.8%
🤖 Hermes automated review: changes requested
Reviewed diff
5014af3...2ff1b8e(+106/-4, 9 files): MinVer git-tag versioning,GET /versionendpoint, startup version log, Dockerfile git install,.dockerignore.git/un-exclusion, anddocker-publish.ymlcontainer-tag workflow changes. ADR 0020 is well written and the overall design is sound. Static security scan clean (no secrets, injection, eval/exec, pickle in added lines). However, the build fails and there is a Docker tag validity problem — both blocking.Major (blocking)
1. Build break:
Assembly.GetCustomAttribute<T>()not in scope — missingusing System.Reflection;backend/src/DoujinManager.Server/Program.cs:94and:133The PR adds code that calls
Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()at two sites (startup log line 94,/versionendpoint line 133).GetCustomAttribute<T>is an extension method defined inCustomAttributeExtensionswithin theSystem.Reflectionnamespace, butProgram.cshas nousing System.Reflection;directive (the code uses the fully-qualifiedSystem.Reflection.AssemblyInformationalVersionAttributetype but never imports the namespace). This produces:(same error at line 133)
Build result: FAILED (2 errors). I verified this by checking out the PR head
2ff1b8eand runningdotnet buildlocally. Addingusing System.Reflection;at the top ofProgram.csresolves both errors and the build succeeds. This also explains why the forgejo-actions coverage bot has posted no comment on this PR —ci.ymlposts coverage only on a successful build+test, and the build is failing.Suggested fix: add
using System.Reflection;to the top ofProgram.cs, or change the calls to use the staticAttribute.GetCustomAttribute(assembly, typeof(...))form.2. MinVer version string contains
+, which is invalid in a Docker image tag.github/workflows/docker-publish.yml:55—git.kagaku.eu/teamai/doujin-manager:${{ steps.version.outputs.version }}MinVer's pre-release output format is
0.1.0-dev.N+sha(e.g.0.1.0-dev.5+abc1234). The+separator introduces SemVer build metadata and is not allowed in a Docker reference tag — valid tag characters are[a-z0-9_.-]only (must also be lowercase, see finding below re: the prior PR #12).docker buildx/ the registry will reject a tag containing+. This means thebuild-and-pushjob will fail the moment MinVer emits a build-metadata segment (which is the default behavior — every non-release commit gets+sha).Suggested fix: strip/sanitize the version before using it as a Docker tag. Either set
MinVerBuildMetadatato empty (suppresses the+sha), or in the workflow step replace+with-(and-already being valid):The PR's own example (
0.1.0-dev.5+abc1234) demonstrates the problematic character is always present for dev builds.Minor (non-blocking)
3.
MinVerDefaultPreReleasePhaseis deprecated in MinVer 6backend/Directory.Build.props:7— MinVer warns:Since this is a brand-new adoption, worth using the non-deprecated
MinVerDefaultPreReleaseIdentifiers=dev.0property now to avoid a future break.4.
/versionendpoint duplicates the reflection logic from the startup logbackend/src/DoujinManager.Server/Program.cs:92-97(startup) and:130-141(/version) both re-fetch theAssemblyInformationalVersionAttributeinline. Consider extracting a smallstatic string GetVersion()helper (or reading it once into a variable/registered singleton) to avoid the duplicated reflection and keep a single source of truth.5.
.dockerignore.git/un-exclusion increases build context size.dockerignore:8— Removing.git/from the ignore list is correct and necessary for MinVer, but the.gitdirectory can be large for long-lived repos. Worth a note that the build context will now carry full git history on everydocker build. Not actionable now, just flagging for awareness.Verification
dotnet build src/DoujinManager.Server/DoujinManager.Server.csproj -c Release→ FAILED (2 CS1061 errors, bothGetCustomAttribute— see finding #1). Confirmed resolved by addingusing System.Reflection;.git diffbetween base5014af3and head2ff1b8e.Note: This is a conversation comment, not a formal Forgejo review approval — the current MCP integration cannot create review approval states or inline diff comments.
Automated daily review. I never merge PRs.
All five findings addressed:
#1 (blocker) — Build break — Already fixed in
8c22d71. Addedusing System.Reflection;.#2 (blocker) — Docker tag
+invalid — Fixed in109926e. The workflow now sanitizes the MinVer output:VERSION="${VERSION/+/-}". Dev builds get tags like0.1.0-dev.5-abc1234instead of+abc1234. Release builds are unaffected (no+in clean versions).#3 (minor) — MinVer deprecation — Already fixed in
8c22d71. Switched toMinVerDefaultPreReleaseIdentifiers=dev.0.#4 (minor) — Duplicated reflection logic — Fixed in
109926e. Extracted to astatic string GetInformationalVersion()helper. Both the startup log and/versionendpoint call the same method — single source of truth.#5 (minor) —
.gitin build context — Acknowledged, no action needed. The repo is small and the build cache layers handle this efficiently.109926e5afc9cf499cb7🤖 Hermes automated review: no blocking issues found (follow-up)
Re-reviewed the 3 fix commits since the prior review (head
2ff1b8e→c9cf499). All 5 prior findings are resolved. The force-update replaced the broken head with a clean, building set of commits.Prior findings — resolution verification
using System.Reflection;(CS1061)1cc8df7using System.Reflection;added atbackend/src/DoujinManager.Server/Program.cs:16;GetCustomAttribute<>now resolves. Confirmed by CI (coverage comment #356 posted for headc9cf499, andci.ymlposts coverage only on successful build+test).+shainvalidcb7d33c.github/workflows/docker-publish.yml:28now runsVERSION="${VERSION/+/-}"afterminver. I verified the substitution:0.1.0-dev.5+abc1234→0.1.0-dev.5-abc1234, and clean versions (0.1.0) are untouched.-is a valid Docker tag char.MinVerDefaultPreReleasePhasedeprecated1cc8df7backend/Directory.Build.props:6now uses<MinVerDefaultPreReleaseIdentifiers>dev.0</MinVerDefaultPreReleaseIdentifiers>(the non-deprecated MinVer 6 property).cb7d33c+c9cf499static string GetInformationalVersion()(Program.cs:148), called by both the startup log and/versionendpoint. Single source of truth.c9cf499additionally moved the method above the top-level-statement boundary to fix a CS8803 warning..gitin build contextStatic security scan
Clean on the 3 fix commits (added lines only): no secrets, no shell injection, no
eval/exec, nopickle, no SQL string formatting.Verification
c9cf499(coverage date 06/29 18:12:15–18:12:29 UTC, generated 18:12:31 UTC; head commitc9cf499timestamp 20:11 per PR). Line coverage 83.6%, branch 49.5%. Sinceci.ymlposts coverage only on a successful build+test, this confirms the build break is fixed and the suite passes. Local build/test skipped per CI-evidence policy.git diff 2ff1b8e...c9cf499on the 3 fix commits, plus full-range7907427...c9cf499for context.docker-publish.ymlworkflow triggers onpush:mainandv*tags only — not onpull_request. So the container-tag sanitization is not exercised by PR CI; it will first run after merge / on tag push. This is a pre-existing workflow-design choice (consistent with PR #22's path filters), not a regression introduced here.Verdict
No blocking or minor issues remain. This is a clean follow-up.
Note: This is a conversation comment, not a formal Forgejo review approval — the current MCP integration cannot create review approval states or inline diff comments.
Automated daily review. I never merge PRs.