Docker lint #12426

kaos · 2021-07-26T17:00:32Z

Continuing docker support, with linting of Dockerfiles, using hadolint.

This will be useful in and of itself, to have a way of linting a projects Dockerfiles, even before having full support for building Docker images with Pants.

Example use:

# src/Dockerfile
FROM base
COPY files /
...

# src/BUILD
docker_image(name="our-image")

$ ./pants lint src/Dockerfile

We need the docker_image in the BUILD file in order to connect the Dockerfile with the hadolint tool, which operates on the sources of docker_image targets.

To configure hadolint, either provide a .hadolint.yaml file in your project root, or point at your preferred config file to use with the [hadolint].config option. (Which becomes --hadolint-config=path/to/config.yaml on the command line.)

benjyw · 2021-07-27T02:40:19Z

Perhaps we should hold off on this until the other PRs have been merged? This can be #4 in the PR breakdown... :)

kaos · 2021-07-27T11:40:12Z

Actually, I think we could sneak this in as PR 2, as it is rather small, and doesn't have any dependencies to the other code besides the DockerImage target type.

If you agree, I'll prepare this one once the first PR is merged.

benjyw · 2021-07-27T13:52:09Z

Sure, sounds OK to me!

Example lint results: $ ./pants lint docker:helloworld 18:54:08.20 [INFO] Completed: lint - hadolint succeeded. ✓ hadolint succeeded. $ ./pants lint docker:helloworld 18:48:00.57 [WARN] Completed: lint - hadolint failed (exit code 1). docker/Dockerfile:2 DL3008 warning: Pin versions in apt get install. Instead of `apt-get install <package>` use `apt-get install <package>=<version>` docker/Dockerfile:2 DL3009 info: Delete the apt-get lists after installing something docker/Dockerfile:2 DL3015 info: Avoid additional packages by specifying `--no-install-recommends` 𐄂 hadolint failed. [ci skip-rust] [ci skip-build-wheels]

kaos · 2021-07-28T16:55:28Z

@benjyw OK, this is ready for review now.

Eric-Arellano

Super neat! A couple things to still do, please:

Move into a dedicated backend/docker/lint/hadolint/ folder with its own register.py (don't forget to add to that init/BUILD). This is really important to v2 that linters are always opt-in, as many people may not want to use it.

You need to register the SkipHadolintField via a PluginField:

pants/src/python/pants/backend/shell/lint/shfmt/skip_field.py

Lines 14 to 18 in 3845df1

    
           def rules(): 
        
               return [ 
        
                   ShellLibrary.register_plugin_field(SkipShfmtField), 
        
                   Shunit2Tests.register_plugin_field(SkipShfmtField), 
        
               ]

The ExternalTool should get a --skip option. In your @rule, return `LintResults([], linter_name="hadolint") if skip is set.
The subsystem should get an --args option w/ type=shell_str. Mix it into the argv.
It'd be great to support config files. Looks like they both do autodiscovery and allow custom locations with --config. Please use the ConfigFiles functionality for this and add the options --config and --config-discovery. See flake8/subsystem.py and flake8/rules.py for an example. (You do not need to have two separate rules and all that partition stuff)
This needs some tests. You can mostly copy pasta {shellcheck,shfmt}/rules_integration_test.py, although also test the --config option like done in flake8/rules_integration_test.py for example.

Generally, adding a linter can mostly be copy pasta from the other linters. I encourage you to look at how the others like shellcheck and shfmt are implemented.

src/python/pants/backend/docker/lint.py

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

src/python/pants/backend/docker/lint/hadolint/rules_integration_test.py

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Super well done! I'm excited for this to land 👏

src/python/pants/backend/docker/lint/hadolint/subsystem.py

src/python/pants/backend/docker/lint/hadolint/rules.py

src/python/pants/backend/docker/lint/hadolint/rules_integration_test.py

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2021-07-29T23:30:09Z

src/python/pants/backend/docker/lint/hadolint/rules.py

+            ),
+        ),
+    )
+    config_files = await Get(ConfigFiles, ConfigFilesRequest, hadolint.config_request())


Ah, sorry, one small perf thing: this can now go into the MultiGet right above.

Eric-Arellano · 2021-07-29T23:31:03Z

src/python/pants/backend/docker/lint/hadolint/rules.py

+    results = [LintResult.from_fallible_process_result(process_result)]
+    return LintResults(results, linter_name="hadolint")


(Subjective style nit that you can feel free to disagree on): might be simpler to inline this, imo the var doesn't add much and adds yet another name to the namespace

Eric-Arellano · 2021-07-29T23:38:09Z

src/python/pants/backend/docker/lint/hadolint/rules.py

@@ -0,0 +1,92 @@
+# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).


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.

Let's worry about that later. We still have to add the core functionality.

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.

Looking at terraform, only the register file is under experimental.. same here?

Yeah that's fine, given how stable we expect this code itself to be. Thanks!

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood requested a review from benjyw July 26, 2021 22:24

kaos changed the title ~~Docker lint~~ WIP: Docker lint Jul 27, 2021

kaos force-pushed the docker_lint branch from e27a726 to ae545fc Compare July 28, 2021 16:54

kaos changed the title ~~WIP: Docker lint~~ Docker lint Jul 28, 2021

kaos requested a review from Eric-Arellano July 28, 2021 16:55

Eric-Arellano reviewed Jul 28, 2021

View reviewed changes

src/python/pants/backend/docker/lint.py Outdated Show resolved Hide resolved

src/python/pants/backend/docker/lint.py Outdated Show resolved Hide resolved

Refactor hadolint tool integration.

3e51f13

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos requested a review from Eric-Arellano July 29, 2021 09:44

Add --hadolint-config option.

ffbda3f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos commented Jul 29, 2021

View reviewed changes

src/python/pants/backend/docker/lint/hadolint/rules_integration_test.py Outdated Show resolved Hide resolved

Replace os.path for PurePath for improved file relation tests.

f52796c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano reviewed Jul 29, 2021

View reviewed changes

kaos added 2 commits July 29, 2021 23:13

drop multiple config file support.

6118eee

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

only discover hadolint config files in project root.

bef4cb7

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Jul 29, 2021

View reviewed changes

Eric-Arellano requested a review from stuhood July 29, 2021 23:28

Eric-Arellano approved these changes Jul 29, 2021

View reviewed changes

Eric-Arellano reviewed Jul 29, 2021

View reviewed changes

kaos added 2 commits July 30, 2021 07:20

final tweaks.

6893cc3

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

move hadolint pluging register module to experimental.

8588126

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano merged commit b44edcb into pantsbuild:main Jul 30, 2021

kaos deleted the docker_lint branch July 30, 2021 07:13

kaos mentioned this pull request Jul 30, 2021

Docker integration #2648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker lint #12426

Docker lint #12426

kaos commented Jul 26, 2021 •

edited

Loading

benjyw commented Jul 27, 2021

kaos commented Jul 27, 2021

benjyw commented Jul 27, 2021

kaos commented Jul 28, 2021

Eric-Arellano left a comment

Eric-Arellano left a comment

Eric-Arellano Jul 29, 2021

Eric-Arellano Jul 29, 2021

Eric-Arellano Jul 29, 2021 •

edited

Loading

benjyw Jul 29, 2021

Eric-Arellano Jul 30, 2021

kaos Jul 30, 2021

Eric-Arellano Jul 30, 2021

	def rules():
	return [
	ShellLibrary.register_plugin_field(SkipShfmtField),
	Shunit2Tests.register_plugin_field(SkipShfmtField),
	]

		results = [LintResult.from_fallible_process_result(process_result)]
		return LintResults(results, linter_name="hadolint")

		@@ -0,0 +1,92 @@
		# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).

Docker lint #12426

Docker lint #12426

Conversation

kaos commented Jul 26, 2021 • edited Loading

benjyw commented Jul 27, 2021

kaos commented Jul 27, 2021

benjyw commented Jul 27, 2021

kaos commented Jul 28, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Jul 29, 2021

Choose a reason for hiding this comment

Eric-Arellano Jul 29, 2021

Choose a reason for hiding this comment

Eric-Arellano Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

benjyw Jul 29, 2021

Choose a reason for hiding this comment

Eric-Arellano Jul 30, 2021

Choose a reason for hiding this comment

kaos Jul 30, 2021

Choose a reason for hiding this comment

Eric-Arellano Jul 30, 2021

Choose a reason for hiding this comment

kaos commented Jul 26, 2021 •

edited

Loading

Eric-Arellano Jul 29, 2021 •

edited

Loading