Conversation
📝 WalkthroughWalkthroughAdds a Makefile providing build, test, coverage, formatting, vet, and CI-check targets; updates AGENTS.md to reference Makefile-driven workflows; and adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
15-15: Deduplicate lint availability checks to reduce drift.
lintandcheck-lintrepeat the samegolangci-lintpresence check; extracting a shared prerequisite keeps behavior consistent and resolves the long-body warning.Refactor sketch
-.PHONY: help build test test-coverage lint fmt vet clean install check-fmt check-vet check-lint ci-checks all +.PHONY: help build test test-coverage lint fmt vet clean install check-fmt check-vet check-lint ci-checks all require-golangci-lint + +require-golangci-lint: + `@if` ! command -v golangci-lint >/dev/null 2>&1; then \ + echo "golangci-lint not found. Install it with: go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest"; \ + exit 1; \ + fi @@ lint: ## Run golangci-lint `@echo` "Running golangci-lint..." - `@if` command -v golangci-lint >/dev/null 2>&1; then \ - golangci-lint run ./...; \ - else \ - echo "golangci-lint not found. Install it with: go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest"; \ - exit 1; \ - fi + @$(MAKE) require-golangci-lint + `@golangci-lint` run ./... @@ check-lint: ## Run golangci-lint and fail on issues (for CI) `@echo` "Running golangci-lint..." - `@if` command -v golangci-lint >/dev/null 2>&1; then \ - golangci-lint run ./... || (echo "golangci-lint found issues. Please fix them."; exit 1); \ - else \ - echo "golangci-lint not found. Install it with: go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest"; \ - exit 1; \ - fi + @$(MAKE) require-golangci-lint + `@golangci-lint` run ./... || (echo "golangci-lint found issues. Please fix them."; exit 1)Also applies to: 51-58, 83-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 15, The Makefile repeats the same golangci-lint existence check in targets like lint and check-lint; create a single phony prerequisite target (e.g., ensure-golangci-lint or check-golangci-lint-bin) that performs the golangci-lint presence/installation check and make both lint and check-lint (and other places that repeat the same block) depend on that target, replacing the duplicated shell-check blocks with the prerequisite so behavior and exit codes remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 92-93: The ci-checks target currently omits linting; update the
Makefile so the ci-checks target depends on check-lint (e.g., change the
dependency list for ci-checks from "check-fmt check-vet test" to include
"check-lint") so the Makefile runs lint gating before printing success; ensure
the target still echoes "All CI checks passed!" after dependencies complete.
---
Nitpick comments:
In `@Makefile`:
- Line 15: The Makefile repeats the same golangci-lint existence check in
targets like lint and check-lint; create a single phony prerequisite target
(e.g., ensure-golangci-lint or check-golangci-lint-bin) that performs the
golangci-lint presence/installation check and make both lint and check-lint (and
other places that repeat the same block) depend on that target, replacing the
duplicated shell-check blocks with the prerequisite so behavior and exit codes
remain identical.
| ci-checks: check-fmt check-vet test ## Run all CI checks | ||
| @echo "All CI checks passed!" |
There was a problem hiding this comment.
ci-checks should include lint gating.
check-lint exists but is not part of ci-checks, so CI can pass while lint errors remain.
Proposed fix
-ci-checks: check-fmt check-vet test ## Run all CI checks
+ci-checks: check-fmt check-vet check-lint test ## Run all CI checks
`@echo` "All CI checks passed!"📝 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.
| ci-checks: check-fmt check-vet test ## Run all CI checks | |
| @echo "All CI checks passed!" | |
| ci-checks: check-fmt check-vet check-lint test ## Run all CI checks | |
| `@echo` "All CI checks passed!" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 92 - 93, The ci-checks target currently omits linting;
update the Makefile so the ci-checks target depends on check-lint (e.g., change
the dependency list for ci-checks from "check-fmt check-vet test" to include
"check-lint") so the Makefile runs lint gating before printing success; ensure
the target still echoes "All CI checks passed!" after dependencies complete.
There was a problem hiding this comment.
the check-lint command has been removed, as there is already a fmt command
There was a problem hiding this comment.
@feloy, I understand your decision, though I wanted to clarify that fmt (formatting) and lint (linting) serve different purposes:
gofmt(used infmt/check-fmt) handles code formatting—whitespace, indentation, bracket placement, etc.golangci-lint(used inlint/check-lint) runs multiple static analysis tools that check for style issues, potential bugs, code complexity, performance problems, and much more beyond just formatting.
For example, golangci-lint can catch things like:
- Unused variables and functions
- Error handling issues
- Security vulnerabilities
- Code complexity and maintainability concerns
- Idiomatic Go patterns
Since you have the check-lint target defined in the Makefile, it's available for developers to run manually, even if it's not part of the automated CI checks. If you later decide to add it to ci-checks, it's just a matter of adding check-lint to the dependency list on line 92.
🧠 Learnings used
Learnt from: CR
Repo: kortex-hub/kortex-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:20:46.491Z
Learning: kortex-cli uses Cobra framework for CLI command structure with entry point at `cmd/kortex-cli/main.go` that calls `pkg/cmd.Execute()`, root command at `pkg/cmd/root.go`, and subcommands in `pkg/cmd/<command>.go`
Learnt from: CR
Repo: kortex-hub/kortex-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:20:46.490Z
Learning: Applies to **/*.go : All Go code should be formatted using `go fmt` tool before committing
Learnt from: CR
Repo: kortex-hub/kortex-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:20:46.490Z
Learning: Applies to pkg/cmd/*.go : Create command files in `pkg/cmd/<command>.go` following Cobra pattern: define a `cobra.Command` variable and register it with `rootCmd.AddCommand()` in the `init()` function
Learnt from: CR
Repo: kortex-hub/kortex-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:20:46.491Z
Learning: Applies to pkg/cmd/*_test.go : Create corresponding test files as `pkg/cmd/<command>_test.go` for each command, covering initialization, execution, and error cases
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
I noticed that copyright is different than the one we used to add
here I can see all rights reserved ?
other podman Desktop files:
/**********************************************************************
- Copyright (C) 2026 Red Hat, Inc.
- Licensed under the Apache License, Version 2.0 (the "License");
- you may not use this file except in compliance with the License.
- You may obtain a copy of the License at
- http://www.apache.org/licenses/LICENSE-2.0
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
- SPDX-License-Identifier: Apache-2.0
***********************************************************************/
|
it introduces go vet command, then, should Agents.md be updated with that ? |
Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Yes, I'm updating the AGENTS.md file to use Makefile commands instead, and add missing commands |
I got it from macadam project (https://github.com/crc-org/macadam/blob/448e6f751e25cb6e19ceb22bbb82134f7c51c941/pkg/machinedriver/driver.go). I can change if necessary |
|
yes I think the header is not expected to be formatted like that |
…t command is present Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
1-3:⚠️ Potential issue | 🟠 MajorAdd the required Apache 2.0 Red Hat header to this Markdown source file.
AGENTS.mdis a*.mdsource file, but it does not include the required Apache License 2.0 Red Hat copyright header (year 2026).As per coding guidelines, "
**/*.{go,md}: All source files must include Apache License 2.0 copyright headers with Red Hat copyright (current year: 2026)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 1 - 3, Add the required Apache License 2.0 Red Hat copyright header (year 2026) to the top of AGENTS.md by inserting the standard multi-line license header used across the repo for *.md files, ensuring it appears before any other content and includes "Copyright Red Hat" with the year 2026 and the Apache-2.0 notice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@AGENTS.md`:
- Around line 1-3: Add the required Apache License 2.0 Red Hat copyright header
(year 2026) to the top of AGENTS.md by inserting the standard multi-line license
header used across the repo for *.md files, ensuring it appears before any other
content and includes "Copyright Red Hat" with the year 2026 and the Apache-2.0
notice.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoreAGENTS.mdMakefile
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- Makefile
OK, I'll fix the headers and the skill: #10 |
2 participants
golangci-lint is not included, as it requires an external command, it will be added in a follow-up PR
Commands for building production binaries are not included, this will be done in a separate PR