Skip to content

[CI] [History Server] Enhance CI ecosystem - #5247

Open
CheyuWu wants to merge 2 commits into
ray-project:masterfrom
CheyuWu:ci/historyserver-golangci-lint
Open

[CI] [History Server] Enhance CI ecosystem#5247
CheyuWu wants to merge 2 commits into
ray-project:masterfrom
CheyuWu:ci/historyserver-golangci-lint

Conversation

@CheyuWu

@CheyuWu CheyuWu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Why are these changes needed?

historyserver/ was the only Go module not covered by the repo-wide golangci-lint setup. Its make alllint target pointed at .golangci.yaml / .golangci.info.yaml config files that don't exist in the module, so it never actually ran.

This PR wires historyserver/ into the shared lint pipeline and fixes all resulting findings:

  • Add historyserver to scripts/lint.sh, so it is linted by the existing golangci-lint pre-commit hook (and the Lint (pre-commit) CI job) with the repo-root .golangci.yml.
  • Replace the broken alllint/issuelint/todolist/install-golint targets with a standard make lint target that installs golangci-lint v2.11.4 (matching .pre-commit-config.yaml) into bin/.
  • Fix the reported issues across the module: %v -> %w on wrapped errors, err == X -> errors.Is, interface{} -> any, import grouping, noctx (new test/support.HTTPGet helper for context-bound requests in e2e tests), testify assertion style, and //nolint:gosec with explanations where the finding is a false positive.
  • Add a few narrow exclusions to the root .golangci.yml: errcheck on ResponseWriter.Write / go-restful Response.Write*, misspell on cancelled (Ray's own spelling in its ErrorType enum and raylet debug state), gosec G117 on SessionKey (a Ray session id, not a credential), and staticcheck SA1019 for aws-sdk-go v1, which the S3 backend still uses.

No behavior changes.

Related issue number

Closes #4813

Labels

  • If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
  • If this PR contains breaking changes, I have added the breaking-change label.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Manual test instructions

cd historyserver
make lint   # 0 issues
make test   # all packages pass

@CheyuWu
CheyuWu marked this pull request as draft September 5, 2026 13:21

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread historyserver/pkg/collector/eventcollector/eventcollector.go Outdated
@CheyuWu

CheyuWu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 82e13bc. Configure here.

}, TestTimeoutMedium).Should(Succeed())
}

func listAzureBlobDirectories(containerClient *container.Client, prefix string) ([]string, error) {

@CheyuWu CheyuWu Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This function is unused, so I remove it

Comment thread historyserver/Makefile
golangci-lint: ## Download golangci-lint locally if necessary.
test -s $(GOLANGCI_LINT) || (curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | bash -s -- -b $(REPO_ROOT_BIN)/ $(GOLANGCI_LINT_VERSION))

.PHONY: issuelint

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This code needed improvement, so I refined it.

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu
CheyuWu force-pushed the ci/historyserver-golangci-lint branch from 82e13bc to 25ae8de Compare September 5, 2026 15:28
@CheyuWu
CheyuWu marked this pull request as ready for review September 5, 2026 15:28
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
Comment on lines +142 to +149
{
name: "/api/cluster_status endpoint (live cluster)",
testFunc: testLiveClusterStatus,
},
{
name: "/api/cluster_status endpoint (dead cluster)",
testFunc: testDeadClusterStatus,
},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I add this back because testLiveClusterStatus and testDeadClusterStatus is unused.

@CheyuWu

CheyuWu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

cc @win5923 PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] [History Server] Enhance build/CI ecosystem

1 participant