Skip to content

Conversation

@ziccardi
Copy link
Contributor

@ziccardi ziccardi commented Nov 25, 2025

This PR contains 2 commits:

  1. Enables the new OCM linter
  2. Fixes the lint issues detected by the new linter

@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Walkthrough

Adds an ocm-lint configuration, enables a custom ocmlogger linter in GolangCI, changes the Makefile lint command to use golangci-lint custom and to run ocm-lint, and bumps the GitHub Action for golangci-lint to v9. Also small logging changes in leadership/flag.go.

Changes

Cohort / File(s) Summary
OCM linter config
./.custom-gcl.yml
New ocm-lint config (version v2.1.6); sets output: ./bin and registers one plugin: module github.com/openshift-online/ocm-sdk-go, import path github.com/openshift-online/ocm-sdk-go/linters, root ..
GolangCI config
./.golangci.yml
Adds a custom linter entry for ocmlogger (type module, descriptive multi-line description) and enables ocmlogger; extends exclusions and adds formatters.exclusions.
Makefile lint target
./Makefile
Replaces golangci-lint run with golangci-lint custom and adds $(LOCAL_BIN_PATH)/ocm-lint run to the lint target.
CI workflow
.github/workflows/check-pull-request.yaml
Updates golangci-lint GitHub Action from golangci/golangci-lint-action@v8 to @v9; step arguments unchanged.
Logging tweaks
leadership/flag.go
Adjusts several log messages to include additional context (process, flag name, error) and on precheck error now lowers the flag and continues (reschedule).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Dev as Developer
  participant Make as Makefile
  participant GolangCI as golangci-lint (local/action)
  participant OcmLint as ocm-lint
  participant Config as .custom-gcl.yml

  Dev->>Make: make lint
  Make->>GolangCI: golangci-lint custom (reads .golangci.yml, loads ocmlogger)
  GolangCI-->>Make: lint results
  Make->>OcmLint: $(LOCAL_BIN_PATH)/ocm-lint run
  OcmLint->>Config: load .custom-gcl.yml (resolve plugin module/import)
  OcmLint-->>Make: ocm-lint results
  Note right of Make: collect and report combined lint output
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify golangci-lint custom mode and ocmlogger module settings in .golangci.yml.
  • Confirm .custom-gcl.yml plugin module/import/root values and ocm-lint version compatibility.
  • Check Makefile change ensures $(LOCAL_BIN_PATH)/ocm-lint is available in CI and local dev.
  • Review leadership/flag.go logging logic and the new precheck behavior (lower + continue).

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Enable the OCM linter' directly matches the main objective of the pull request, which is to enable the new OCM linter across the codebase.
Description check ✅ Passed The description accurately describes the two main components of the PR: enabling the OCM linter and fixing lint issues detected by it, which aligns with the actual changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9de1920 and e95b248.

📒 Files selected for processing (3)
  • .custom-gcl.yml (1 hunks)
  • .golangci.yml (1 hunks)
  • Makefile (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Test (1.21, macos-latest)
  • GitHub Check: Test (1.21, ubuntu-latest)
  • GitHub Check: Test (1.21, windows-latest)
🔇 Additional comments (1)
.custom-gcl.yml (1)

1-10: Manual verification required for ocm-lint v2.1.6 configuration and plugin accessibility.

Unable to definitively confirm from codebase or public sources:

  1. ocm-lint v2.1.6 availability — Web search yielded no clear results confirming this version exists as a published release
  2. Plugin module accessibility — No references to github.com/openshift-online/ocm-sdk-go/linters as a plugin endpoint were found in the repository
  3. GCL configuration format — No .custom-gcl.yml files or GCL framework references exist in the codebase, suggesting this may be a new external tool

The review's path resolution concern (line 5: ./bin vs $(LOCAL_BIN_PATH)) is valid and should be verified to ensure consistent behavior across different working directory contexts.

Recommend: Test the configuration locally by running the tool with this configuration file to confirm version availability, plugin module resolution, and path behavior before merging.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
leadership/flag.go (1)

323-333: Precheck error handling/logging improved; consider aligning context usage.

Including process, flag-name, and err in the precheck failure log is a clear improvement, and the combination of schedule(ctx, f.retryInterval) + lower(ctx) on both error and !check paths makes the flag state explicit and conservative.

One minor consistency point: here you log with f.ctx, while the rest of this method and file mostly use the locally derived ctx that adds the leadershipFlag value. Using ctx instead of f.ctx would keep log context uniform and ensure any values injected into ctx are visible to the logger as well.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 361f3e8 and a057f3d.

📒 Files selected for processing (5)
  • .custom-gcl.yml (1 hunks)
  • .github/workflows/check-pull-request.yaml (1 hunks)
  • .golangci.yml (1 hunks)
  • Makefile (1 hunks)
  • leadership/flag.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/workflows/check-pull-request.yaml
  • .custom-gcl.yml
  • .golangci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Test (1.21, macos-latest)
  • GitHub Check: Test (1.21, ubuntu-latest)
  • GitHub Check: Test (1.21, windows-latest)
  • GitHub Check: Generate
  • GitHub Check: Lint
🔇 Additional comments (3)
Makefile (1)

97-101: Lint target chaining looks correct; please confirm ocm-lint provisioning.

The updated recipe correctly chains golangci-lint custom and ocm-lint run so ocm-lint only executes on a successful custom lint run, and any failure in golangci-lint --version will still fail the lint target.

Please just verify that your .custom-gcl.yml / build flow reliably produces or installs $(LOCAL_BIN_PATH)/ocm-lint in both local and CI environments; otherwise make lint will fail on a missing binary.

leadership/flag.go (2)

385-393: Including the error in the create-state log is helpful.

The updated format string for the “create initial state” failure now logs the underlying error, which will make diagnosing database issues much easier. Arguments match the placeholders correctly.


466-470: Update-holder error log now has full context; looks good.

Similarly, the “update holder” error path now includes the error value alongside process and flag name, which aligns with other error logs in this type and improves observability. Placeholder/argument usage is correct.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2025
@openshift-ci
Copy link

openshift-ci bot commented Nov 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oriAdler, ziccardi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2025
@oriAdler
Copy link
Contributor

I verified the changes locally:

make lint 
golangci-lint --version
golangci-lint has version (devel) built with go1.24.7 from (unknown, modified: ?, mod sum: "") on (unknown)
golangci-lint custom && \
/home/oadler/github.com/openshift-online/ocm-sdk-go/bin/ocm-lint run
0 issues.

@openshift-merge-bot openshift-merge-bot bot merged commit 04dbb14 into openshift-online:main Nov 26, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants