Fixing print of lint errors in golangci-lint action

[patrickhuie19]

Previously the lint artifact was in xml, so the step to find errors omits the file name when logging to the action. This means that devs still have to pull the artifact down locally to fix lint errors, which I think is exactly what the step was trying to prevent.

Example: https://github.com/smartcontractkit/chainlink-relay/actions/runs/6896336069/job/18762232176?pr=248

I saw two paths forward:

• Change the output encoding to json, which is easier to parse than xml
• Write a more complex parser for the xml to include the file name with the error

I chose the first, with the caveat that golangci-lint allows multiple output encodings/files, which is necessary as sonar only allows for golangci-lint artifacts with the checkstyle format: https://docs.sonarsource.com/sonarqube/latest/analyzing-source-code/importing-external-issues/external-analyzer-reports/#list-of-properties

The main thing we have to make sure of here is that no integration with sonar is broken. Verified with https://github.com/smartcontractkit/chainlink-relay/pull/246/checks?check_run_id=18763287499 on commit 332877966b03c1556be6cc2ee82721a8063f0cce of this PR, which has parity with the same lint errors on https://github.com/smartcontractkit/chainlink-relay/pull/248

[jmank88]

Previously the lint artifact was in xml, so the step to find errors omits the file name when logging to the action.

What do you mean? They are grouped under file tags with a name attribute. For example:

<?xml version="1.0" encoding="UTF-8"?>

<checkstyle version="5.0">
  <file name="pkg/chainlink/config/config.go">
    <error column="23" line="51" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="52" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="53" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="27" line="60" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="20" line="63" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="22" line="66" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="8" line="78" message="SA1019: utils.URL is deprecated: use config.URL " severity="error" source="staticcheck"></error>
  </file>
</checkstyle>

But regardless, we have shared actions for the whole organization defined here: https://github.com/smartcontractkit/.github We should be using that, and fixing the problem there for everyone.

[jmank88]

This means that devs still have to pull the artifact down locally to fix lint errors, which I think is exactly what the step was trying to prevent.

They are supposed to show up in the summary, and in-line as annotations too, but those things often break or are truncated, and you have to wait for the full workflow to complete. I don't know why this specific logic was added - we usually just print the file :shrug:

[jmank88]

I tried to color the output but for some reason installing bat was not trivial.

• https://github.com/smartcontractkit/chainlink/pull/9266

[patrickhuie19]

Previously the lint artifact was in xml, so the step to find errors omits the file name when logging to the action.

What do you mean? They are grouped under file tags with a name attribute. For example:

<?xml version="1.0" encoding="UTF-8"?>

<checkstyle version="5.0">
  <file name="pkg/chainlink/config/config.go">
    <error column="23" line="51" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="52" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="53" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="27" line="60" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="20" line="63" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="22" line="66" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="8" line="78" message="SA1019: utils.URL is deprecated: use config.URL " severity="error" source="staticcheck"></error>
  </file>
</checkstyle>

But regardless, we have shared actions for the whole organization defined here: smartcontractkit/.github We should be using that, and fixing the problem there for everyone.

I provided a direct example of how the lint action behaves today in the PR description. The step that prints to the terminal hunts for errors and not the file they are grouped under.

[patrickhuie19]

Previously the lint artifact was in xml, so the step to find errors omits the file name when logging to the action.

What do you mean? They are grouped under file tags with a name attribute. For example:

<?xml version="1.0" encoding="UTF-8"?>

<checkstyle version="5.0">
  <file name="pkg/chainlink/config/config.go">
    <error column="23" line="51" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="52" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="23" line="53" message="SA1019: utils.Duration is deprecated: use config.Duration " severity="error" source="staticcheck"></error>
    <error column="27" line="60" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="20" line="63" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="22" line="66" message="SA1019: utils.MustNewDuration is deprecated: use config.MustNewDuration " severity="error" source="staticcheck"></error>
    <error column="8" line="78" message="SA1019: utils.URL is deprecated: use config.URL " severity="error" source="staticcheck"></error>
  </file>
</checkstyle>

But regardless, we have shared actions for the whole organization defined here: smartcontractkit/.github We should be using that, and fixing the problem there for everyone.

This repo stands on its own and has higher standards than core or other product repos. I'm fine with that the .github repo suggesting best practices, but ultimately CI/CD should empower the devs who own a repo to maintain it.

Do we have automation to enforce that and automatically fix deviations? I'd prefer to fix it first, and then ask releng to do a survey of where all the repos are at and clean up inconsistencies. I'd be opening up two PRs anyways.

[jmank88]

There is an ongoing effort to standardize CI across the organization that we are not excluded from and that we should be contributing to instead of deviating inside of one repo.

[jmank88]

The step that prints to the terminal hunts for errors and not the file they are grouped under.

The idea that errors need to be searched for within the file is misguided.

[patrickhuie19]

There is an ongoing effort to standardize CI across the organization that we are not excluded from and that we should be contributing to instead of deviating inside of one repo.

Is there a document capturing this effort? Were devs asked for feedback?

Best practices are awesome, along with some type of CLI tooling to create a new repo from scratch that incorporates these best practices into the first commit.

I'm happy to open an exact analog of this PR across .github, but the current behavior has been bugging me for a while, is an anti-pattern to what the step desires, and I fixed it. Why are we blocking ourselves from moving to a better state?

[jmank88]

Is there a document capturing this effort? Were devs asked for feedback?

What do you mean? We have an active repo https://github.com/smartcontractkit/.github which is where changes should be made by default.

Why are we blocking ourselves from moving to a better state?

Why are we fixing something that we don't mean to be using in the first place, instead of dropping in the thing that we do mean to use by default?