Skip to content
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

Updates lint output to print detail instead of description #5045

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

daghack
Copy link
Collaborator

@daghack daghack commented Jun 17, 2024

Currently, the PrintLintViolations output warning text includes the static description of the lint rule, rather than the formatted text message that often includes context specific to a given instance of rule violation.

This updates PrintLintViolations to print out the formatted lint message instead. Below is an instance where you can see the difference in the types of output.

Current:

UndefinedVar - https://docs.docker.com/go/dockerfile/rule/undefined-var/
Variables should be defined before their use
Dockerfile:3
--------------------
   1 |     FROM alpine
   2 | >>> ENV PATH=$PAHT:/foo
   3 |
--------------------

New:

UndefinedVar - https://docs.docker.com/go/dockerfile/rule/undefined-var/
Usage of undefined variable '$PAHT' (did you mean $PATH?)
Dockerfile:3
--------------------
   1 |     FROM alpine
   2 | >>> ENV PATH=$PAHT:/foo
   3 |
--------------------

@daghack daghack self-assigned this Jun 17, 2024
@daghack daghack added this to the v0.14.1 milestone Jun 17, 2024
@daghack daghack marked this pull request as draft June 17, 2024 14:16
@daghack daghack force-pushed the detail-not-description branch 2 times, most recently from 1de23db to 71854d5 Compare June 17, 2024 14:36
@daghack daghack marked this pull request as ready for review June 17, 2024 15:01
@daghack daghack requested a review from tonistiigi June 17, 2024 15:34
@tonistiigi
Copy link
Member

We probably need to go over these more in some follow-ups. Looking at the example I would say in that case both messages are useful. One is pointing out a mistake and suggesting a fix and the other is describing what the general rule for the validation is. I guess for some of the rules the separation isn't quite as obvious and we don't show both to avoid duplication. In that case I think we can define what rules benefit from multiple fields and keep it hidden/empty if extra field does not add additional context. We can also define more clearly how the verbosity changes with --debug.

@tonistiigi
Copy link
Member

This removes the line position from the default output for build warnings (without --debug).

Before:

 1 warning found (use --debug to expand):
 - UndefinedVar: Usage of undefined variable '$PAHT' (did you mean $PATH?) (line 2)

Now:

 1 warning found (use --debug to expand):
 - UndefinedVar: Usage of undefined variable '$PAHT' (did you mean $PATH?)

@daghack daghack marked this pull request as draft June 17, 2024 19:23
Signed-off-by: Talon Bowler <talon.bowler@docker.com>
@daghack
Copy link
Collaborator Author

daghack commented Jun 17, 2024

Updated to return the line number to the buildkit progress stream and docker --debug output.

@daghack daghack marked this pull request as ready for review June 17, 2024 20:13
func LintFormatShort(rulename, msg string, line int) string {
msg = fmt.Sprintf("%s: %s", rulename, msg)
if line > 0 {
msg = fmt.Sprintf("%s (line %d)", msg, line)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonistiigi Is it needed to have the line number in progress as we already have this information in?:

Usage of undefined variable '$PAHT' (did you mean $PATH?)
Dockerfile:3
--------------------
   1 |     FROM alpine
   2 | >>> ENV PATH=$PAHT:/foo
   3 |
--------------------

Mostly looking at docker/actions-toolkit#365 (comment)

image

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is only with --debug. By default there is no source code view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is not needed for example in your github actions excerpt. It should probably be added in buildx side for views that need it, not inside the string here.

@crazy-max crazy-max merged commit 5deda4a into moby:master Jun 18, 2024
75 checks passed
@daghack daghack deleted the detail-not-description branch June 24, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants