feat: add hadolint linting step to container workflow#2028
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds hadolint-based linting for the repository Containerfile, integrating it into both the GitHub Actions container workflow and the local pre-commit hook, with a global .hadolint.yaml configuration and supporting documentation/metadata updates.
Changes:
- Added a new
.hadolint.yamlto centrally configure ignored hadolint rules (with rationale). - Added a hadolint lint step to
.github/workflows/container.yamland tocontrib/dev-tools/git/hooks/pre-commit.sh(only whenContainerfileis staged). - Updated documentation and metadata conventions (
semantic-links, issue draft spec, and spellchecker dictionary).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
project-words.txt |
Adds “Dockerfiles” to the project dictionary for spellcheck consistency. |
docs/skills/semantic-skill-link-convention.md |
Documents semantic-links placement patterns and adds the related-artifacts marker. |
docs/issues/drafts/1460-add-hadolint-to-container-workflow.md |
Draft spec describing the hadolint workflow/config approach and policy. |
contrib/dev-tools/git/hooks/pre-commit.sh |
Adds a hadolint step gated on staged Containerfile changes. |
Containerfile |
Adds a semantic-links comment referencing .hadolint.yaml for discoverability. |
.hadolint.yaml |
New global hadolint ignore configuration with rationale for ignored rules. |
.github/workflows/container.yaml |
Adds a hadolint lint step before Buildx setup/build steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2028 +/- ##
===========================================
+ Coverage 81.50% 81.61% +0.11%
===========================================
Files 342 342
Lines 24337 24337
Branches 24337 24337
===========================================
+ Hits 19835 19862 +27
+ Misses 4202 4177 -25
+ Partials 300 298 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.hadolint.yaml:43
- The DL4006 rationale currently states the
curl | bashURL is “pinned” and that acurl --failguard is not used, but the Containerfile uses a moving.../main/...URL and already passes-sSf(includes--fail). This makes the rationale misleading and also doesn’t clearly explain the real risk of ignoringpipefailforcurl | bashpipelines.
# - `curl ... | bash`: downloads a trusted install script from a pinned
# HTTPS URL (raw.githubusercontent.com/cargo-bins/cargo-binstall). The
# source is verified via TLS 1.2 and the URL is controlled by the project
# we depend on. While `curl | bash` is high-impact, the risk is mitigated
# by the signed HTTPS transport and the trusted upstream. A `curl --fail`
…ecks/ Move format-project-words.sh from contrib/dev-tools/git/ to contrib/dev-tools/checks/ and extract the inline hadolint command from pre-commit.sh into a standalone lint-containerfile.sh sensor. Update pre-commit.sh to reference the checks/ scripts instead of the old paths. Both sensor scripts now compute PROJECT_ROOT from their new location (../../.. instead of ../.., since they moved one directory deeper relative to the repo root). Part of the sensor/harness architecture design under EPIC torrust#2003, tracked in issue torrust#1460.
32f1883 to
da1b4f3
Compare
…ve test alongside sensor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
contrib/dev-tools/checks/tests/test-format-project-words.sh:6
- PROJECT_ROOT is computed three directories up from contrib/dev-tools/checks/tests/, which resolves to contrib/dev-tools instead of the repository root. This makes the subsequent cp paths ("${PROJECT_ROOT}/contrib/dev-tools/...") incorrect and will break these tests.
This issue also appears on line 14 of the same file.
contrib/dev-tools/checks/tests/test-format-project-words.sh:23
- The pre-commit hook now invokes ./contrib/dev-tools/checks/lint-containerfile.sh, but the test fixture only copies format-project-words.sh. This will cause the pre-commit invocation in these tests to fail with “No such file or directory”. Also, the hadolint sensor expects to run inside a git work tree, so the fixture should initialize an empty repo so
git diff --cachedcan run deterministically.
…files Add header comments linking: - format-project-words.sh to its test file with manual run reminder - lint-containerfile.sh with note that no automated tests exist yet - test-format-project-words.sh to its sensor with manual run reminder All reference EPIC torrust#2003 for the long-term automation harness redesign.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
contrib/dev-tools/checks/tests/test-format-project-words.sh:13
PROJECT_ROOTis computed with only three..segments, which resolves to thecontrib/directory (not the repository root). That makes subsequent paths like${PROJECT_ROOT}/contrib/dev-tools/...incorrect (they becomecontrib/contrib/...) and breaks the test fixture setup when run manually.
contrib/dev-tools/git/hooks/pre-commit.sh:31- This change moves the dictionary formatter invocation to
contrib/dev-tools/checks/format-project-words.sh, but the old pathcontrib/dev-tools/git/format-project-words.shno longer exists (and is still referenced from multiple docs/skills). Without either updating those references or providing a small wrapper at the old path, contributors following the docs will hit a broken command.
declare -a STEPS=(
"Formatting project dictionary|./contrib/dev-tools/checks/format-project-words.sh"
"Checking for unused dependencies (cargo machete --with-metadata)|cargo machete --with-metadata"
…DL3008 rationale, redundant skill-link
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
contrib/dev-tools/checks/tests/test-format-project-words.sh:13
PROJECT_ROOTis computed as three directories up fromcontrib/dev-tools/checks/tests, which resolves to<repo>/contribrather than the repository root. That makes subsequent paths like${PROJECT_ROOT}/contrib/dev-tools/...expand to<repo>/contrib/contrib/...and breaks these integration tests when run manually.
This issue also appears on line 26 of the same file.
contrib/dev-tools/checks/tests/test-format-project-words.sh:30
- The fixture copies
pre-commit.sh, but the hook now includes the hadolint step (./contrib/dev-tools/checks/lint-containerfile.sh). In this test fixture there is nolint-containerfile.sh, so the manual integration testit_should_continue_pre_commit_checks_when_dictionary_is_already_formattedwill fail when it reaches that step.
|
ACK 3d98c36 |
Description
Closes #1460
Adds a hadolint Dockerfile linting step to the
container.yamlGitHub Actions workflow, ensuring theContainerfilemeets Docker best practices.Changes
.hadolint.yaml(new): Global hadolint configuration with documented rationale for 4 systematically ignored rules (DL3008, DL3059, DL4006, SC2046).github/workflows/container.yaml: Added hadolint step before setup-buildx, mounting the config fileContainerfile: Added semantic-links comment referencing.hadolint.yamlfor discoverabilitycontrib/dev-tools/git/hooks/pre-commit.sh: Added hadolint step that runs only if Containerfile changeddocs/issues/drafts/1460-add-hadolint-to-container-workflow.md: Updated spec to reflect the global config approachIgnore Policy
.hadolint.yamlwith documented rationale# hadolint ignore=comments with rationaleVerification
linter allpassescargo test --doc --workspacepasses