Add linter for Dockerfile[.in] and GHA for it #5210

rucoder · 2025-09-05T13:43:29Z

Description

We have a lot of Dockerfile files that use outdated syntaf for ENV and VAR assignments. This PR introduces a tool that can be used both as linter and and also can fix all Dockefile's automatically

UPDATE 1: I decided got for tree-sitter based linter

How to test and validate this PR

no need to test from EVE perspective but the package has tests in Python

I also added a dummy Dockerfile to test GH annotations

PR Backports

- 14.5-stable: Up to maintainers 
- 13.4-stable: Up to maintainers

Checklist

I've provided a proper description
I've added the proper documentation
I've tested my PR on amd64 device
I've tested my PR on arm64 device
I've written the test verification instructions
I've set the proper labels to this PR

And the last but not least:

I've checked the boxes above, or I've provided a good reason why I didn't
check them.

Copilot

Pull Request Overview

This PR introduces a comprehensive Dockerfile linter tool that detects and fixes outdated ENV/ARG syntax in Dockerfile files. The tool modernizes syntax by adding proper = assignment operators and quotes where necessary, ensuring compliance with modern Docker best practices.

Adds a Python-based linter for Dockerfile and Dockerfile.in files
Includes GitHub Actions workflow for automated CI checks
Provides comprehensive test coverage with unit tests

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tools/ci/dockerfile_linter/dockerfile_linter.py	Main implementation of the Dockerfile linter with CLI support
tools/ci/dockerfile_linter/test_dockerfile_linter.py	Comprehensive unit tests for all linter functionality
tools/ci/dockerfile_linter/requirements.txt	Dependencies file (currently empty as tool uses only stdlib)
tools/ci/dockerfile_linter/init.py	Package initialization file with metadata
tools/ci/dockerfile_linter/main.py	Module entry point for python -m execution
tools/ci/dockerfile_linter/README.md	Detailed documentation and usage examples
tools/ci/dockerfile_linter/.gitignore	Git ignore rules for Python cache files
.github/workflows/dockerfile-check.yml	GitHub Actions workflow for automated syntax checking

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-05T14:05:11Z

tools/ci/dockerfile_linter/test_dockerfile_linter.py

+            self.linter.process_directory(str(self.test_dir))
+
+            # Should process files
+            self.assertEqual(self.linter.files_processed, 1)  # Only finds Dockerfile, not Dockerfile.good


The comment is incorrect. Based on the find_dockerfiles logic, it should find files named 'Dockerfile' and 'Dockerfile.in', but 'Dockerfile.good' would not match these patterns and should not be counted.

Copilot · 2025-09-05T14:05:11Z

tools/ci/dockerfile_linter/dockerfile_linter.py

+        env_match = re.match(r'^(ENV)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)
+        arg_match = re.match(r'^(ARG)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)


The regex patterns for ENV and ARG variable names are overly restrictive. Docker allows lowercase letters, periods, and hyphens in variable names. The patterns should be [A-Za-z_][A-Za-z0-9_.-]* to match actual Docker variable naming conventions.

Suggested change

env_match = re.match(r'^(ENV)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)

arg_match = re.match(r'^(ARG)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)

env_match = re.match(r'^(ENV)\s+([A-Za-z_][A-Za-z0-9_.-]*)\s+(.+)$', stripped)

arg_match = re.match(r'^(ARG)\s+([A-Za-z_][A-Za-z0-9_.-]*)\s+(.+)$', stripped)

Copilot · 2025-09-05T14:05:12Z

tools/ci/dockerfile_linter/dockerfile_linter.py

+            for i, value in enumerate(all_values):
+                if i == len(all_values) - 1:
+                    # Last line - no continuation backslash
+                    modified_lines.append(f"    {value}\"")
+                else:
+                    # Continuation line
+                    modified_lines.append(f"    {value} \\")


The multiline formatting adds extra indentation (4 spaces) but doesn't preserve the original indentation from the input. This could change the visual formatting of carefully formatted Dockerfiles. Consider preserving or detecting the original indentation pattern.

Suggested change

for i, value in enumerate(all_values):

if i == len(all_values) - 1:

# Last line - no continuation backslash

modified_lines.append(f" {value}\"")

else:

# Continuation line

modified_lines.append(f" {value} \\")

# Detect indentation from the first continuation line, if available

indent = ""

if start_idx + 1 < len(lines):

match = re.match(r"^(\s*)", lines[start_idx + 1])

if match:

indent = match.group(1)

if indent == "":

indent = " " # fallback to 4 spaces if no indentation detected

for i, value in enumerate(all_values):

if i == len(all_values) - 1:

# Last line - no continuation backslash

modified_lines.append(f"{indent}{value}\"")

else:

# Continuation line

modified_lines.append(f"{indent}{value} \\")

Copilot · 2025-09-05T14:05:12Z

.github/workflows/dockerfile-check.yml

+                      **/Dockerfile.in
+
+            - name: Set up Python
+              uses: actions/setup-python@v4


The actions/setup-python action should use the latest version v5 instead of v4 for security updates and improved functionality.

Suggested change

uses: actions/setup-python@v4

uses: actions/setup-python@v5

christoph-zededa · 2025-09-05T14:27:44Z

tools/ci/dockerfile_linter/dockerfile_linter.py

+            return False
+
+        # Check if value contains spaces or other characters that need quoting
+        return bool(re.search(r'[\s\$\`\|\&\;\(\)\<\>\{\}\[\]\*\?]', value))


Python has quotemeta, too: https://docs.python.org/3/library/re.html#re.escape

- it can be also used to fix issue found. See README.md Signed-off-by: Mikhail Malyshev <[email protected]>

- we run on changed files only - it should generate output that GH understands Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

deitch · 2025-09-08T06:24:57Z

In principle, I like this idea. We should gave Dockerfiles (and everything else) up to date in syntax.

I have two areas of question.

First, how does this fit with the other GHA CI paths we follow? Should this be part of one of the others? We have a fairly complex structure of CI in GHA, maybe we should check with @yash-zededa ?

Second, this sounds like it fits in line with what yetus does? For all of our complaints about yetus, it does a good job centralizing all of the various linters and checkers into a single place. Should this be part of that? Is there a yetus option or plugin that can do this?

europaul

just some nitpicking

europaul · 2025-09-09T08:24:11Z

tools/ci/dockerfile_linter/dockerfile_linter.py

+def main():
+    ap = argparse.ArgumentParser(description="Dockerfile linter (Tree-sitter only) for modernizing ENV/ARG.")
+    ap.add_argument('directory', nargs='?', default='.', help='Directory to process (default: .)')
+    ap.add_argument('--ci', action='store_true', help='CI mode: lint only, do not modify files; nonzero exit on issues')


tbh I'm not a big fan of using flag names such as ci or github-actions - when looking at those I have absolutely no idea what they mean practically and need to always go look at the help messages to figure it out. Since there is no dark magic happening in the background I would just call this one something like --lint-only or --check-syntax or --only-warn or --fail-on-syntax-warnings

europaul · 2025-09-09T08:33:21Z

.github/workflows/dockerfile-check.yml

+                  pip install -r tools/ci/dockerfile_linter/requirements.txt
+                  pip install pytest
+
+            - name: Lint changed Dockerfiles


Since the script does the reformatting automatically, wouldn't it be better to just apply it to all Dockerfiles that we have, check them in and then on every CI run check all the files again?

Or do you fear rebuilding all packages again? 😄

@europaul i definitively will run it on eve tree after this PR is merged

rene · 2025-09-23T10:10:12Z

@rucoder , it looks like the Linter is proven to work, so you can remove the fake-test commit in order to finalize the review and allow the PR to get merged...

rucoder · 2025-09-23T10:17:33Z

@rucoder , it looks like the Linter is proven to work, so you can remove the fake-test commit in order to finalize the review and allow the PR to get merged...

@rene I'm not sure about one case: ARG MYARG= seems to be a valid syntax, I need to check

rucoder requested review from deitch and yash-zededa as code owners September 5, 2025 13:43

github-actions bot requested review from eriknordmark, jsfakian and uncleDecart September 5, 2025 13:43

christoph-zededa requested a review from Copilot September 5, 2025 14:04

Copilot AI reviewed Sep 5, 2025

View reviewed changes

christoph-zededa reviewed Sep 5, 2025

View reviewed changes

rucoder marked this pull request as draft September 6, 2025 11:35

rucoder force-pushed the rucider/dockerfile-linter branch from 2209f90 to 2873a7c Compare September 6, 2025 13:47

rucoder marked this pull request as ready for review September 6, 2025 14:00

rucoder added 3 commits September 6, 2025 16:25

Add a linter for Dockerfile

9c23182

- it can be also used to fix issue found. See README.md Signed-off-by: Mikhail Malyshev <[email protected]>

Add workflow to run Dockerfile linter

868b85d

- we run on changed files only - it should generate output that GH understands Signed-off-by: Mikhail Malyshev <[email protected]>

[DO NOT MERGE] dummy Dockerfile to test linter GHA annotations

f31b0b3

Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder force-pushed the rucider/dockerfile-linter branch from f281c34 to f31b0b3 Compare September 6, 2025 16:25

rucoder added the side-quest A worthy adventure, but not essential for victory. Tackle when the main quest is safe! label Sep 7, 2025

europaul approved these changes Sep 9, 2025

View reviewed changes

		env_match = re.match(r'^(ENV)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)
		arg_match = re.match(r'^(ARG)\s+([A-Z_][A-Z0-9_]*)\s+(.+)$', stripped)

Add linter for Dockerfile[.in] and GHA for it #5210

Are you sure you want to change the base?

Add linter for Dockerfile[.in] and GHA for it #5210

Uh oh!

Conversation

rucoder commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test and validate this PR

PR Backports

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

christoph-zededa Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

deitch commented Sep 8, 2025

Uh oh!

europaul left a comment

Choose a reason for hiding this comment

Uh oh!

europaul Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

europaul Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

rucoder Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

rene commented Sep 23, 2025

Uh oh!

rucoder commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rucoder commented Sep 5, 2025 •

edited

Loading