ci: add golangci-lint tooling - #1359
Conversation
Add ARCHITECTURE.md documenting the codebase map, package layout, core sign/verify workflows, trust model, and developer workflow, linked from README.md and building.md. Add golangci-lint tooling: a conservative .golangci.yml adopted incrementally via new-from-rev, a 'lint' Makefile target, and a CI lint step (with fetch-depth: 0 so new-from-rev gating has git history). Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1359 +/- ##
=======================================
Coverage 79.09% 79.09%
=======================================
Files 68 68
Lines 3076 3076
=======================================
Hits 2433 2433
Misses 436 436
Partials 207 207 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds an architecture overview document and introduces additive Go lint tooling/CI wiring to improve contributor onboarding and maintain code quality without enforcing the existing lint backlog.
Changes:
- Add
ARCHITECTURE.mdand link it fromREADME.mdandbuilding.md. - Add a
make linttarget and a golangci-lint v2 configuration. - Add a golangci-lint GitHub Actions step and adjust checkout depth for new-issues gating.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Adds a top-level link to the new architecture overview document. |
Makefile |
Introduces a lint target for running golangci-lint locally. |
building.md |
Links to ARCHITECTURE.md for codebase structure context. |
ARCHITECTURE.md |
New architecture and workflow documentation for the codebase. |
.golangci.yml |
Adds golangci-lint v2 configuration intended for incremental adoption. |
.github/workflows/build.yml |
Adds CI lint step and ensures git history is available for new-issues gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
new-from-rev: HEAD compares the tree against itself, producing an empty diff so no PR-introduced issues are ever reported. Switch to new-from-merge-base: origin/main so lint issues introduced by a PR are caught while the pre-existing backlog stays unenforced. Update the build.yml checkout comment and ARCHITECTURE.md to match. Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
- Remove package-layout tables; defer symbol-level detail to godoc. - Convert ASCII diagrams to mermaid (high-level, sign/verify, CI/CD). - Remove hard line-wrapping in prose per markdown preference. - Remove the Improvement Opportunities section; such items belong in roadmaps/issues rather than architecture docs. Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
- Redesign all mermaid diagrams for readability: flat top-down layered tree for the high-level architecture (no cramped nested subgraphs), simple left-to-right pipelines for sign/verify workflows, and a two-lane CI/CD diagram. Verified rendering via mermaid-cli. - Add a callout to the Trust Model section pointing to notaryproject/specifications as the source of truth; this doc is a practical implementation summary, not a restatement of the spec. Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
Color-code each layer (CLI, CLI-internal, shared utils, external libs), bold the package paths, and label edges (delegates to / built on / calls) so relationships are explicit rather than implied by bare arrows. Verified rendering locally with mermaid-cli before pushing. Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
|
Hiya team, thank you so much for all the thoughtful review so far, it's genuinely helped tighten this up! Given there is no release happened for this repo for a while. ❤️ One thought on scope: if it makes reviewing easier, I'm happy to split this PR so the golangci-lint tooling ( Totally happy to go either way, just let me know what you'd prefer and I'll action it right away. Thanks again for your guidance here! |
Per review discussion, split scope: keep this PR limited to the golangci-lint tooling (.golangci.yml, make lint, CI wiring) and move the architecture overview to a separate follow-up PR for its own focused review. Signed-off-by: Tatsat Mishra <tamishra@microsoft.com>
|
Update: went ahead and split this out — removed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.golangci.yml:53
go.moddeclares the module path asgithub.com/notaryproject/notation/v2, but goimportslocal-prefixesis set togithub.com/notaryproject/notation. This can cause goimports to treat this module's own imports as third-party and reorder them unexpectedly.
- github.com/notaryproject/notation
shizhMSFT
changed the title
Description
This PR is scoped to golangci-lint developer/CI tooling only — additive, non-breaking, no source, CLI, or dependency changes.
(Note: this PR originally also included an
ARCHITECTURE.mddoc; per review discussion, that was split out into its own follow-up PR so this one stays focused and easy to review.)Developer / CI tooling (additive, non-breaking)
.golangci.yml(golangci-lint v2) with a conservative linter set (bodyclose,misspell,revive,unconvert,whitespace+gofmt/goimportsformatters).issues.new-from-merge-base: origin/main, so only issues introduced by a PR (relative to its merge base withmain) fail CI — the existing lint backlog is not enforced.make linttarget.build.yml(golangci-lint-actionv8, golangci-lint v2.5.0), and setfetch-depth: 0on checkout so merge-base gating has the git history it needs to diff against.Compatibility
go 1.24.0and fully supports this module's declaredgo 1.24.0toolchain — no version mismatch.actions/setup-gobefore the lint step runs, sogolangci-linttype-checks against that same toolchain.Why it is safe
cmd/orinternal/— no behavior, API, or CLI-flag changes.go.mod/dependency or Go toolchain changes.new-from-merge-base), so it cannot fail on the pre-existing backlog (~76 pre-existing findings remain and are unaffected).Signed commits ✅