-
-
Notifications
You must be signed in to change notification settings - Fork 638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docker lint #12426
Merged
Merged
Docker lint #12426
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ae545fc
Add hadolint to validate Dockerfiles.
kaos 3e51f13
Refactor hadolint tool integration.
kaos ffbda3f
Add --hadolint-config option.
kaos f52796c
Replace os.path for PurePath for improved file relation tests.
kaos 6118eee
drop multiple config file support.
kaos bef4cb7
only discover hadolint config files in project root.
kaos 6893cc3
final tweaks.
kaos 8588126
move hadolint pluging register module to experimental.
kaos File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
python_library() |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
python_library() | ||
python_tests(name="tests") |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from pants.backend.docker.lint.hadolint import skip_field | ||
from pants.backend.docker.lint.hadolint.rules import rules as hadolint_rules | ||
|
||
|
||
def rules(): | ||
return ( | ||
*hadolint_rules(), | ||
*skip_field.rules(), | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
from __future__ import annotations | ||
|
||
from dataclasses import dataclass | ||
from pathlib import PurePath | ||
|
||
from pants.backend.docker.lint.hadolint.skip_field import SkipHadolintField | ||
from pants.backend.docker.lint.hadolint.subsystem import Hadolint | ||
from pants.backend.docker.target_types import DockerImageSources | ||
from pants.core.goals.lint import LintRequest, LintResult, LintResults | ||
from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest | ||
from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest | ||
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest | ||
from pants.engine.fs import Digest, MergeDigests | ||
from pants.engine.platform import Platform | ||
from pants.engine.process import FallibleProcessResult, Process | ||
from pants.engine.rules import Get, MultiGet, collect_rules, rule | ||
from pants.engine.target import FieldSet, Target | ||
from pants.engine.unions import UnionRule | ||
from pants.util.logging import LogLevel | ||
from pants.util.strutil import pluralize | ||
|
||
|
||
@dataclass(frozen=True) | ||
class HadolintFieldSet(FieldSet): | ||
required_fields = (DockerImageSources,) | ||
|
||
sources: DockerImageSources | ||
|
||
@classmethod | ||
def opt_out(cls, tgt: Target) -> bool: | ||
return tgt.get(SkipHadolintField).value | ||
|
||
|
||
class HadolintRequest(LintRequest): | ||
field_set_type = HadolintFieldSet | ||
|
||
|
||
@rule(desc="Lint with Hadolint", level=LogLevel.DEBUG) | ||
async def run_hadolint(request: HadolintRequest, hadolint: Hadolint) -> LintResults: | ||
if hadolint.skip: | ||
return LintResults([], linter_name="Hadolint") | ||
|
||
downloaded_hadolint, sources = await MultiGet( | ||
Get(DownloadedExternalTool, ExternalToolRequest, hadolint.get_request(Platform.current)), | ||
Get( | ||
SourceFiles, | ||
SourceFilesRequest( | ||
[field_set.sources for field_set in request.field_sets], | ||
for_sources_types=(DockerImageSources,), | ||
enable_codegen=True, | ||
), | ||
), | ||
) | ||
config_files = await Get( | ||
ConfigFiles, ConfigFilesRequest, hadolint.config_request(sources.snapshot.dirs) | ||
) | ||
input_digest = await Get( | ||
Digest, | ||
MergeDigests( | ||
( | ||
sources.snapshot.digest, | ||
downloaded_hadolint.digest, | ||
config_files.snapshot.digest, | ||
) | ||
), | ||
) | ||
|
||
# As hadolint uses a single config file, we need to partition our runs per config file | ||
# discovered. | ||
files_with_config = _group_files_with_config( | ||
sources.snapshot.files, | ||
config_files.snapshot.files, | ||
not hadolint.config, | ||
) | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
processes = [ | ||
Process( | ||
argv=[downloaded_hadolint.exe, *config, *hadolint.args, *files], | ||
input_digest=input_digest, | ||
description=f"Run `hadolint` on {pluralize(len(files), 'Dockerfile')}.", | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
level=LogLevel.DEBUG, | ||
) | ||
for files, config in files_with_config | ||
] | ||
process_results = await MultiGet(Get(FallibleProcessResult, Process, p) for p in processes) | ||
results = [ | ||
LintResult.from_fallible_process_result(process_result) | ||
for process_result in process_results | ||
] | ||
return LintResults(results, linter_name="hadolint") | ||
|
||
|
||
def _group_files_with_config( | ||
source_files: tuple[str, ...], config_files: tuple[str, ...], config_files_discovered: bool | ||
) -> list[tuple[tuple[str, ...], list[str]]]: | ||
"""If config_files_discovered, group all source files that is in the same directory or below a | ||
config file, otherwise, all files will be kept in one group per config file that was provided as | ||
option.""" | ||
groups = [] | ||
consumed_files: set[str] = set() | ||
source_paths = {PurePath(source_file) for source_file in source_files} | ||
|
||
for config_file in config_files: | ||
if not config_files_discovered: | ||
files = set(source_files) | ||
else: | ||
config_path = PurePath(config_file).parent | ||
files = {str(source) for source in source_paths if config_path in source.parents} | ||
if files: | ||
# Sort files, to make the order predictable for the tests. | ||
groups.append((tuple(sorted(files)), ["--config", config_file])) | ||
consumed_files.update(files) | ||
|
||
if len(consumed_files) < len(source_files): | ||
files = set(source_files) - consumed_files | ||
groups.append((tuple(files), [])) | ||
|
||
return groups | ||
|
||
|
||
def rules(): | ||
return [ | ||
*collect_rules(), | ||
UnionRule(LintRequest, HadolintRequest), | ||
] |
150 changes: 150 additions & 0 deletions
150
src/python/pants/backend/docker/lint/hadolint/rules_integration_test.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,150 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import annotations | ||
|
||
from textwrap import dedent | ||
|
||
import pytest | ||
|
||
from pants.backend.docker.lint.hadolint.rules import HadolintFieldSet, HadolintRequest | ||
from pants.backend.docker.lint.hadolint.rules import rules as hadolint_rules | ||
from pants.backend.docker.target_types import DockerImage | ||
from pants.core.goals.lint import LintResult, LintResults | ||
from pants.core.util_rules import config_files, external_tool, source_files | ||
from pants.engine.addresses import Address | ||
from pants.engine.target import Target | ||
from pants.testutil.rule_runner import QueryRule, RuleRunner | ||
|
||
|
||
@pytest.fixture | ||
def rule_runner() -> RuleRunner: | ||
return RuleRunner( | ||
rules=[ | ||
*hadolint_rules(), | ||
*config_files.rules(), | ||
*external_tool.rules(), | ||
*source_files.rules(), | ||
QueryRule(LintResults, [HadolintRequest]), | ||
], | ||
target_types=[DockerImage], | ||
) | ||
|
||
|
||
GOOD_FILE = dedent( | ||
""" | ||
FROM python:3.8 | ||
""" | ||
) | ||
|
||
BAD_FILE = dedent( | ||
""" | ||
FROM python | ||
""" | ||
) | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
def run_hadolint( | ||
rule_runner: RuleRunner, targets: list[Target], *, extra_args: list[str] | None = None | ||
) -> tuple[LintResult, ...]: | ||
rule_runner.set_options( | ||
["--backend-packages=pants.backend.docker.lint.hadolint", *(extra_args or ())], | ||
env_inherit={"PATH"}, | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
results = rule_runner.request( | ||
LintResults, | ||
[HadolintRequest(HadolintFieldSet.create(tgt) for tgt in targets)], | ||
) | ||
return results.results | ||
|
||
|
||
def assert_success( | ||
rule_runner: RuleRunner, target: Target, *, extra_args: list[str] | None = None | ||
) -> None: | ||
result = run_hadolint(rule_runner, [target], extra_args=extra_args) | ||
assert len(result) == 1 | ||
assert result[0].exit_code == 0 | ||
assert not result[0].stdout | ||
assert not result[0].stderr | ||
|
||
|
||
def test_passing(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files({"Dockerfile": GOOD_FILE, "BUILD": "docker_image(name='t')"}) | ||
tgt = rule_runner.get_target(Address("", target_name="t")) | ||
assert_success(rule_runner, tgt) | ||
|
||
|
||
def test_failing(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files({"Dockerfile": BAD_FILE, "BUILD": "docker_image(name='t')"}) | ||
tgt = rule_runner.get_target(Address("", target_name="t")) | ||
result = run_hadolint(rule_runner, [tgt]) | ||
assert len(result) == 1 | ||
assert result[0].exit_code == 1 | ||
assert "Dockerfile:2 " in result[0].stdout | ||
|
||
|
||
def test_multiple_targets(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files( | ||
{ | ||
"Dockerfile.good": GOOD_FILE, | ||
"Dockerfile.bad": BAD_FILE, | ||
"BUILD": dedent( | ||
""" | ||
docker_image(name="good", sources=("Dockerfile.good",)) | ||
docker_image(name="bad", sources=("Dockerfile.bad",)) | ||
""" | ||
), | ||
} | ||
) | ||
tgts = [ | ||
rule_runner.get_target( | ||
Address("", target_name="good", relative_file_path="Dockerfile.good") | ||
), | ||
rule_runner.get_target(Address("", target_name="bad", relative_file_path="Dockerfile.bad")), | ||
] | ||
result = run_hadolint(rule_runner, tgts) | ||
assert len(result) == 1 | ||
assert result[0].exit_code == 1 | ||
assert "Dockerfile.good" not in result[0].stdout | ||
assert "Dockerfile.bad:2 " in result[0].stdout | ||
|
||
|
||
def test_config_files(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files( | ||
{ | ||
"a/Dockerfile": BAD_FILE, | ||
"a/BUILD": "docker_image()", | ||
"a/.hadolint.yaml": "ignored: [DL3006]", | ||
"b/Dockerfile": BAD_FILE, | ||
"b/BUILD": "docker_image()", | ||
} | ||
) | ||
tgts = [ | ||
rule_runner.get_target(Address("a")), | ||
rule_runner.get_target(Address("b")), | ||
] | ||
result = run_hadolint(rule_runner, tgts) | ||
# We get two runs of hadolint, for the `a` and `b` directories respectively. | ||
Eric-Arellano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert len(result) == 2 | ||
assert result[0].exit_code == 0 | ||
assert "a/Dockerfile" not in result[0].stdout | ||
assert "b/Dockerfile:2 " not in result[0].stdout | ||
assert result[1].exit_code == 1 | ||
assert "a/Dockerfile" not in result[1].stdout | ||
assert "b/Dockerfile:2 " in result[1].stdout | ||
|
||
tgt = rule_runner.get_target(Address("b")) | ||
assert_success(rule_runner, tgt, extra_args=["--hadolint-config=a/.hadolint.yaml"]) | ||
|
||
|
||
def test_passthrough_args(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files({"Dockerfile": BAD_FILE, "BUILD": "docker_image(name='t')"}) | ||
tgt = rule_runner.get_target(Address("", target_name="t")) | ||
assert_success(rule_runner, tgt, extra_args=["--hadolint-args=--ignore DL3006"]) | ||
|
||
|
||
def test_skip(rule_runner: RuleRunner) -> None: | ||
rule_runner.write_files({"Dockerfile": BAD_FILE, "BUILD": "docker_image(name='t')"}) | ||
tgt = rule_runner.get_target(Address("", target_name="t")) | ||
result = run_hadolint(rule_runner, [tgt], extra_args=["--hadolint-skip"]) | ||
assert not result |
64 changes: 64 additions & 0 deletions
64
src/python/pants/backend/docker/lint/hadolint/rules_test.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
import pytest | ||
|
||
from pants.backend.docker.lint.hadolint.rules import _group_files_with_config | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"source_files, config_files, config_files_discovered, expected", | ||
[ | ||
( | ||
{"a/Dockerfile", "b/Dockerfile"}, | ||
("a/.hadolint.yaml",), | ||
True, | ||
[ | ||
(("a/Dockerfile",), ["--config", "a/.hadolint.yaml"]), | ||
(("b/Dockerfile",), []), | ||
], | ||
), | ||
( | ||
{"a/Dockerfile", "b/Dockerfile"}, | ||
("hadolint.yaml",), | ||
False, | ||
[ | ||
(("a/Dockerfile", "b/Dockerfile"), ["--config", "hadolint.yaml"]), | ||
], | ||
), | ||
( | ||
{"a/Dockerfile", "aa/Dockerfile"}, | ||
("a/.hadolint.yaml",), | ||
True, | ||
[ | ||
(("a/Dockerfile",), ["--config", "a/.hadolint.yaml"]), | ||
(("aa/Dockerfile",), []), | ||
], | ||
), | ||
( | ||
{"a/Dockerfile", "b/Dockerfile", "c/Dockerfile", "d/Dockerfile", "c/e/Dockerfile"}, | ||
("a/.hadolint.yaml", "c/.hadolint.yaml"), | ||
True, | ||
[ | ||
(("a/Dockerfile",), ["--config", "a/.hadolint.yaml"]), | ||
( | ||
( | ||
"c/Dockerfile", | ||
"c/e/Dockerfile", | ||
), | ||
["--config", "c/.hadolint.yaml"], | ||
), | ||
( | ||
( | ||
"b/Dockerfile", | ||
"d/Dockerfile", | ||
), | ||
[], | ||
), | ||
], | ||
), | ||
], | ||
) | ||
def test_group_files_with_config(source_files, config_files, config_files_discovered, expected): | ||
actual = _group_files_with_config(source_files, config_files, config_files_discovered) | ||
assert actual == expected |
15 changes: 15 additions & 0 deletions
15
src/python/pants/backend/docker/lint/hadolint/skip_field.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from pants.backend.docker.target_types import DockerImage | ||
from pants.engine.target import BoolField | ||
|
||
|
||
class SkipHadolintField(BoolField): | ||
alias = "skip_hadolint" | ||
default = False | ||
help = "If true, don't run hadolint on this target's Dockerfile." | ||
|
||
|
||
def rules(): | ||
return [DockerImage.register_plugin_field(SkipHadolintField)] |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh didn't realize Docker is still under an experimental backend. This linter should be aligned with the normal Docker backend.
@benjyw do you think it should be experimental still? Now that we have Hadolint, Docker support is a Real Thing, more than just
./pants tailor
. For example, our Shell support is only linters + a test runner.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's worry about that later. We still have to add the core functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. @kaos can you please rename this folder to be experimental then? Note that this backend doesn't work without first activating the core Docker backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at terraform, only the register file is under
experimental
.. same here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine, given how stable we expect this code itself to be. Thanks!