LegacyKeyValueFormat linter is noisy with very little practical value

[tianon]

At the risk of being a broken record, I still firmly believe it is a mistake to even attempt to deprecate the ENV key value syntax in favor of only supporting ENV key=value. Parsing the former is much simpler, and more in line with the way other Dockerfile instructions are parsed (and have been parsed historically). It is also much easier to generate from other code correctly and safely. As I noted on that other issue, if there are cases where the behavior of the former is ambiguous, that should be the thing we detect and warn about, not just trying to deprecate the entire syntax.

For the current linting rule, I would propose that it either be removed or somehow disabled by default, as it is causing unnecessary and noisy churn across the industry with very little practical benefit. 🙇 ❤️

Examples of unnecessary churn/noise:

• Update "ENV key value" format in README krallin/tini#226
• feat: Update Dockerfile ENV instructions to use = instead of whitespace as the key value separator nginxinc/docker-nginx#906
• Update Bonita base alpine image version to 3.20 docker-library/official-images#17136
• Supress LegacyKeyValueFormat warnings docker-library/ruby#465
• eggheads/eggdrop-docker@e685e40
• remove legacy ENV variables adoptium/containers#628
• Dockerfile: Fix legacy ENV key value format docker/docker-ce-packaging#1047
• style: update the use of LegacyKeyValueFormat in Dockerfiles carlossg/docker-maven#487
• [master] rpm, deb: fix Dockerfile linting issues docker/docker-ce-packaging#1064

[tonistiigi]

The syntax is deprecated and discouraged (since 2020). We should either remove the deprecation or we should warn. I don't see any point of deprecating but then still saying "it is fine, keep using it". If there are many Dockerfiles that use the deprecated syntax 4 years later, that is a sign that nobody follows the deprecation documentation, and if we want to change user's behavior, we need to tell them that they are using a deprecated command.

Personally, I tend to agree with the justification for deprecation. In addition to ambiguous cases:

• there shouldn't be a need to have multiple ways to define the same thing. Most people learn Dockerfile from reading other Dockerfiles, not by reading docs.
• space delimiters confuse users about possibility to define multiple env with the same command
• it is inconsistent with variable definitions in shell and export what most of Dockerfile commands are modeled after
• it is more consistent with ARG that doesn't support space delimiters

as it is causing unnecessary and noisy churn across the industry

It is only shown until you fix it one time. There is a #check=skip= directive in top of the file if you want to take a risk of using deprecated syntax.

@colinhemmings

[tianon]

Yes, I agree with your first point -- I argued against the deprecation back in 2020, and still do think we should reverse course on it because I still think it's a net negative.

"It's more consistent with ARG" isn't really compelling to me given that ENV came first and ARG was intentionally designed in a different way, which I also disagree with.

The way I see the "multiple ways to do the same thing" argument is no different than shell vs JSON syntax. It's a little hiccup for new users, but experienced users learn quickly. I do think there is value in having a warning for cases like ENV foo bar baz=buzz where it is clearly ambiguous, and I think that's a better use of a linting warning.

[tonistiigi]

The way I see the "multiple ways to do the same thing" argument is no different than shell vs JSON syntax.

But we did add a check for that as well. While non-JSON is allowed from CMD/ENTRYPOINT we now warn against it and ask user to only use JSON.

[tianon]

And RUN and VOLUME and COPY? (in all of which JSON vs non-JSON each have useful use cases)

[Nefcanto]

I recommend that you change this lint's word order. First specifying the problem and then presenting the solution.

I have mentioned it in another issue moby/moby#48327 (comment)

[thaJeztah]

Copying from the other ticket (which was discussing format of the error-message);

Using active voice would probably be good ("use X" instead of "X should be used").

Perhaps more along the lines how we format Deprecated: messages in our Go codebase, e.g.;

LegacyKeyValueFormat: The "ENV key value" format is deprecated. Use "ENV key value" format instead.

Not sure where to fit in the line 15 though;

LegacyKeyValueFormat: The "ENV key value" format is deprecated. Use "ENV key value" format instead. (line 15)

I'm used to linters pointing out the issue inline, but not sure if possible, and not sure if that could make the output more noisy / harder to read if it's in more complex parts of a Dockerfile;

Dockerfile:15: The "ENV key value" format is deprecated. Use "ENV key value" format instead (LegacyKeyValueFormat).
ENV NODE_ENV production
    ^

I guess that's the intent of the current output, which may work well for a single error, but with the "lines before / after" becomes quite verbose, and also puts a lot of separation between the error and the line containing the error / issue.

[Whapow]

The deprecation error needs to at least be limited to actual Docker ENV lines, currently hitting false-positives within RUN commands:

[thaJeztah]

currently hitting false-positives within RUN commands:

Yes, that's a bug; it's tracked in #5240, and I saw a patch is being worked on (but not yet complete)

[tianon]

I don't think this is the appropriate place to discuss rewording the linter notice -- I'm calling for removal or disabling-by-default of it, and don't think rewording it is sufficient.

[mcandre]

Parser ambiguity is never a good thing. The latest docker build... behavior is useful in encouraging engineers to migrate away from risky legacy syntax. I hope hadolint et all follow suit, as linter suites and CI/CD pipelines tend to run more frequently overall than image builds.

The Docker logs and documentation do a terrible job of explaining the reason for the deprecation. The historical parser ambiguity context is essential, but it is hidden in the GitHub issues.

[thompson-shaun]

The Docker logs and documentation do a terrible job of explaining the reason for the deprecation. The historical parser ambiguity context is essential, but it is hidden in the GitHub issues.

Would we typically link to this kind of information in the docs @dvdksn?

[thaJeztah]

Something like https://staticcheck.dev/docs/checks or https://securego.io/docs/rules/g103 could be nice it gives a page with context about each rule (not sure how "SEO friendly" the current Dockerfile rules are to find them through Google).

[tonistiigi]

Something like https://staticcheck.dev/docs/checks or https://securego.io/docs/rules/g103 could be nice it gives a page with context about each rule (not sure how "SEO friendly" the current Dockerfile rules are to find them through Google).

https://docs.docker.com/reference/build-checks/ are docs for each rule.

[thompson-shaun]

The check doc is the top result via google atm 🥇

I was mostly curious if the GitHub issue context was worth adding to the check's page for historical purposes.

[dvdksn]

I'm not gonna send readers to GitHub issues where people argue about the pros and cons of this syntax. It's deprecated, don't use it.

[mcandre]

I don't care one whit about SEO. I'm asking for the CLI to explain why it bothers to drop support for the old syntax. No reason is given for a seemingly arbitrary warning.

A link from stderr to the relevant documentation page would be sufficient to clear up the confusion.

[thompson-shaun]

Links to the warning documentation are added when using docker buildx build with --debug. That being said, it might be good for us to link to the list of checks when --debug isn't being used.

Thanks for the feedback @mcandre -- I'll create a separate issue for any changes related to the output changes and link it here so this issue stays focused on its original purpose.

[tianon]

The so-called "legacy" syntax is only legacy because when ARG was added, it was added with a syntax that was different (intentionally) from the existing ENV syntax, so I don't really think "parser ambiguity" is a fair complaint here given that it wasn't ambiguous until the ARG foo=bar syntax was added. Again, however, I think a linting warning explicitly for the ambiguous cases is much more fruitful than deprecating the entire "legacy" syntax (which has some strong upsides in its favor) and trying to shift every usage such that we can remove it (especially, again, because there are upsides to the older syntax).