Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .custom-gcl.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
version: v2.8.0
name: custom-gcl
plugins:
- module: github.com/jakedoublev/pathconcat
version: v0.3.0
15 changes: 9 additions & 6 deletions .github/workflows/checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,16 @@ jobs:
name: govulncheck-failure-${{ strategy.job-index }}
path: /tmp/govulncheck-failure-${{ strategy.job-index }}.txt
retention-days: 1
- name: install golangci-lint
run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.8.0
- name: build custom golangci-lint
run: golangci-lint custom
Comment on lines +90 to +93
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Build custom-gcl once per workflow and reuse across matrix jobs.

Compiling the custom binary in every matrix leg adds avoidable CI time. Consider a dedicated prep job that builds custom-gcl once and shares it via artifact/cache.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/checks.yaml around lines 90 - 93, Replace per-matrix-step
compilation by adding a dedicated prep job that builds the custom golangci-lint
binary once and publishes it as an artifact (e.g., "custom-gcl"); then change
the matrix jobs that currently run the steps named "install golangci-lint" and
"build custom golangci-lint" to instead download that artifact (or restore from
cache) before running linting. Ensure the prep job runs before the matrix (use
needs or job dependencies), upload the built binary with upload-artifact, and in
each matrix job use download-artifact (or cache) to reuse the same "custom-gcl"
binary rather than rebuilding in every leg.

- name: golangci-lint
uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 # v8.0.0
with:
version: v2.8.0
working-directory: ${{ matrix.directory }}
skip-cache: true
only-new-issues: true
run: >-
${{ github.workspace }}/custom-gcl run
-c ${{ github.workspace }}/.golangci.yaml
${{ github.event_name == 'pull_request' && format('--new-from-rev={0}', github.event.pull_request.base.sha) || '' }}
working-directory: ${{ matrix.directory }}
- if: matrix.directory == 'service'
run: .github/scripts/init-temp-keys.sh
- run: go test ./... -short
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ tmp-gen/
/examples/examples
/**/kas-*.pem
/opentdf
/custom-gcl
/sdkjava/target
/serviceapp
/service/opentdf
Expand Down
10 changes: 10 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ linters:
- nolintlint
- nonamedreturns
- nosprintfhostport
- pathconcat
- perfsprint
- predeclared
- promlinter
Expand All @@ -73,6 +74,15 @@ linters:
- wastedassign
- whitespace
settings:
custom:
pathconcat:
type: module
description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath"
settings:
ignore-strings:
- "/attr/"
- "/value/"
- "/obl/"
Comment on lines +77 to +85
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add rationale comments for broad ignore-strings patterns.

Line 82–85 uses substring suppressions that are intentionally broad; a short inline note will reduce future accidental edits/regressions.

📝 Suggested clarity update
     custom:
       pathconcat:
         type: module
         description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath"
         settings:
+          # Suppress known FQN token patterns that are not filesystem/URL path construction.
           ignore-strings:
             - "/attr/"
             - "/value/"
             - "/obl/"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
custom:
pathconcat:
type: module
description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath"
settings:
ignore-strings:
- "/attr/"
- "/value/"
- "/obl/"
custom:
pathconcat:
type: module
description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath"
settings:
# Suppress known FQN token patterns that are not filesystem/URL path construction.
ignore-strings:
- "/attr/"
- "/value/"
- "/obl/"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.golangci.yaml around lines 77 - 85, Add short inline rationale comments
explaining why the three broad ignore-strings ("/attr/", "/value/", "/obl/")
exist so future editors don't remove or change them inadvertently; update the
.golangci.yaml custom -> pathconcat -> settings -> ignore-strings block to
include one-line comments for each pattern describing what cases they suppress
(e.g., internal API paths, known URL fragments, or legacy routes) and a note
about the risk of narrowing them, referencing the keys "custom", "pathconcat",
"settings", and "ignore-strings" so reviewers can find the explanation easily.

errcheck:
check-type-assertions: true
exhaustive:
Expand Down
9 changes: 6 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# make
# To run all lint checks: `LINT_OPTIONS= make lint`

.PHONY: all build clean connect-wrapper-generate docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck
.PHONY: all build clean connect-wrapper-generate custom-gcl docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The custom-gcl target should not be marked as .PHONY. Since it produces a binary file named custom-gcl, marking it as .PHONY forces the custom linter to be rebuilt every time make lint or make go-lint is called. Building a custom golangci-lint binary is a relatively slow process as it involves compiling plugins, so avoiding unnecessary rebuilds will improve developer productivity and CI efficiency.

.PHONY: all build clean connect-wrapper-generate docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck


MODS=protocol/go lib/ocrypto lib/fixtures lib/flattening lib/identifier sdk service examples
HAND_MODS=lib/ocrypto lib/fixtures lib/flattening lib/identifier sdk service examples
Expand Down Expand Up @@ -56,11 +56,14 @@ proto-lint:
exit $$exit_code; \
fi)

go-lint:
custom-gcl:
golangci-lint custom
Comment on lines +59 to +60
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure the custom linter binary is rebuilt when its configuration changes, add .custom-gcl.yml as a dependency for the custom-gcl target. This, combined with removing it from .PHONY, ensures the binary is only built when necessary. Additionally, consider adding custom-gcl to your clean target (though it is outside the current diff) to ensure the binary is removed during cleanup.

custom-gcl: .custom-gcl.yml
	golangci-lint custom


go-lint: custom-gcl
status=0; \
for m in $(HAND_MODS); do \
echo "Linting module: $$m"; \
(cd "$$m" && golangci-lint run $(LINT_OPTIONS) ) || status=1; \
(cd "$$m" && $(ROOT_DIR)/custom-gcl run $(LINT_OPTIONS) ) || status=1; \
done; \
exit $$status

Expand Down
Loading