ci: add Docker lint workflow (hadolint + shellcheck)#72
Conversation
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: Project relevance gate passed; Docker lint CI is directly relevant to octo-admin, which has a root Dockerfile and Docker publish workflow.
π¬ Non-blocking
π‘ Warning β .github/workflows/docker-lint.yml: The reusable workflow currently exists on Mininglamp-OSS/.github#61, but not on .github@main. If this PR merges before that dependency, matching events will fail because reusable-docker-lint.yml@main cannot be resolved. The PR description already calls this out, so this is a merge-order caveat rather than a code blocker.
π΅ Suggestion β .github/workflows/docker-lint.yml: Consider matching the repositoryβs existing reusable-workflow caller pattern by adding explicit least-privilege permissions, for example permissions: {} at workflow level and contents: read on the docker-lint job. The called workflow already constrains permissions, so this is consistency hardening rather than required behavior.
β Highlights
The trigger paths match the stated scope: root Dockerfile*, docker/**, Hadolint config, and the workflow file itself. The caller is minimal and aligns with the organizationβs reusable-workflow approach already used elsewhere in this repo.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #72 (octo-admin)
Summary
This PR adds a thin caller workflow (.github/workflows/docker-lint.yml, 21 lines) that delegates to the org-level reusable workflow Mininglamp-OSS/.github/.github/workflows/reusable-docker-lint.yml@main. The path filters, trigger config, and delegation pattern are all correct and idiomatic.
However, there is a blocking dependency-ordering problem: the referenced reusable workflow does not yet exist on main.
Blocking issue (P1)
The pinned reusable workflow does not exist on @main.
Mininglamp-OSS/.github/.github/workflows/reusable-docker-lint.ymlreturns 404 onmain. It is introduced by a separate, still-open PR (Mininglamp-OSS/.github#61, currentlyOPEN/mergeStateStatus: BLOCKED).- As a direct consequence, the Docker Lint workflow run on this PR's head SHA (
935258a) failed β the run completed with 0 jobs (conclusion: failure), the classic signature of GitHub being unable to resolve auses:reference. - If this PR merges before the reusable workflow lands on
main, every push/PR that touchesDockerfile*onocto-adminwill produce a red Docker Lint check onmainuntil the dependency is merged.
This is purely a merge-ordering issue β the PR content is correct, but it cannot be merged independently.
Required change
Land Mininglamp-OSS/.github#61 first (so reusable-docker-lint.yml@main resolves), then re-run CI on this PR and confirm the Docker Lint check passes before merging. No code change to this PR is needed once the dependency is on main.
Non-blocking observations
- Dockerfile is clean. The repo's single
Dockerfile(multi-stagenode:18-alpineβnginx:alpine) has no constructs that would trip hadolint at the defaultwarningthreshold, so once the reusable workflow resolves the hadolint job should pass. - shellcheck is a no-op here. There is no
docker/directory and no*.shfiles in the repo, so the reusable workflow's shellcheck job will self-skip (skip=true). That's expected and fine β it just means this PR currently only exercises the hadolint half. - Reusable workflow itself looks solid (reviewed via
.github#61): least-privilegepermissions: {}at top with per-jobcontents: read,persist-credentials: false, pinned action SHAs, and asha256sum -cintegrity check on the hadolint binary download. No concerns to surface back to this PR.
Verdict
Requesting changes solely to enforce merge ordering: do not merge this PR until reusable-docker-lint.yml is present on Mininglamp-OSS/.github@main and the Docker Lint check passes here.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #72 (octo-admin)
Verdict: APPROVED
A clean, minimal caller workflow that wires this repo into the org's reusable Docker lint pipeline. No blocking issues found.
Verification
| Item | Status | Evidence |
|---|---|---|
| Reusable workflow exists | β | Mininglamp-OSS/.github/.github/workflows/reusable-docker-lint.yml is present on main |
| Stated dependency merged | β | Mininglamp-OSS/.github#61 (the reusable workflow definition) is MERGED, so @main resolves to a real workflow_call target |
| Path-filter correctness | β | Dockerfile*, docker/**, .hadolint.yaml, and the workflow file itself are all covered on both push and pull_request to main (.github/workflows/docker-lint.yml:4-17) |
| hadolint will actually run | β | Repo has a root Dockerfile; the reusable workflow's auto discovery (find . -maxdepth 1 -name 'Dockerfile*') will pick it up |
| shellcheck degrades gracefully | β | No docker/ directory in this repo; the reusable workflow checks [ ! -d "$SHELL_DIR" ] and skips cleanly, so this job is a no-op rather than a failure |
| Permissions | β | The reusable workflow declares permissions: {} at top level and grants only contents: read per job; the caller correctly adds nothing extra |
| Supply-chain hygiene of callee | β | Inside the reusable workflow, actions/checkout and ludeeus/action-shellcheck are pinned to commit SHAs and hadolint is downloaded with a sha256sum -c integrity check |
Findings
No P0/P1 issues.
Nits / suggestions (non-blocking)
uses:pins to@main(.github/workflows/docker-lint.yml:21). Referencing a reusable workflow by mutable branch means a future change toreusable-docker-lint.ymlreaches this repo without review. Pinning to a tag or commit SHA would be stricter supply-chain hygiene. This matches the documented org convention in the reusable workflow's own usage example, so it is acceptable as-is β flagging only as a future hardening option..hadolint.yamlin the path filter (.github/workflows/docker-lint.yml:8,15) β there is currently no.hadolint.yamlin the repo, so hadolint runs with default rules. This is harmless (the filter is forward-looking for when a config is added) and worth keeping.
Additional observations (out of scope for this PR)
- The repo's root
Dockerfileuses floating base tags (node:18-alpine,nginx:alpine). hadolint at the defaultwarningthreshold may surfaceDL3007-style advisories on first run. This is pre-existing and not introduced by this PR; the new lint job is exactly what will start surfacing such items going forward.
Overall this is a low-risk, well-structured CI addition that follows the established reusable-workflow pattern. Approving.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: The PR is relevant to octo-admin because the repo has a root Dockerfile, and the new workflow adds scoped Docker-related CI coverage without introducing blocking issues.
π¬ Non-blocking
- π‘ Warning: .github/workflows/docker-lint.yml uses workflow-level path filters. This is fine for an optional lint workflow, but it should not be configured as a required branch-protection check unless the project accepts skipped/missing checks for PRs that do not touch Docker paths.
- π΅ Suggestion: .github/workflows/docker-lint.yml could explicitly declare least-privilege permissions at the caller job level, for example
contents: read, to match the repoβs general CI hardening style. This is not blocking because the called reusable workflow is expected to handle its own permissions.
β Highlights
- π΅ The workflow is correctly scoped to root
Dockerfile*,docker/**,.hadolint.yaml, and its own workflow file. - π΅ The reusable workflow call syntax is consistent with the existing org-level reusable workflow pattern in this repository.
- π΅ No application logic, secrets, deployment behavior, or runtime code paths are changed.
What
Add
docker-lint.ymlcaller workflow that invokes the organization'sreusable-docker-lint.ymlto lint Dockerfiles (hadolint) and shellscripts in
docker/(shellcheck).How it works
Dockerfile*at repo root. Skips gracefullyif none found.
./docker/for.shfiles. Skips gracefully ifdirectory doesn't exist or has no scripts.
Dependency
Requires Mininglamp-OSS/.github#61 to be merged first (the reusable
workflow definition).
Ref: Mininglamp-OSS workflow optimization T11.