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

output: Add the gitlab output. #220

Merged
merged 5 commits into from
Nov 21, 2024
Merged

output: Add the gitlab output. #220

merged 5 commits into from
Nov 21, 2024

Conversation

octo
Copy link
Contributor

@octo octo commented Nov 20, 2024

This adds a new output format, gitlab, which produces Code Quality reports for GitLab. This allows GitLab CI jobs to annotate which files need formatting. Without this, a CI job can only fail, leaving the user to sieve through the logs to find out which files they need to format.

Example output:

[
  {
    "description": "Not formatted correctly, run yamlfmt to resolve.",
    "check_name": "yamlfmt",
    "fingerprint": "c1dddeed9a8423b815cef59434fe3dea90d946016c8f71ecbd7eb46c528c0179",
    "severity": "major",
    "location": {
      "path": ".golangci.yaml"
    }
  }
]

This adds a new output format, `gitlab`, which produces [Code Quality
reports](https://docs.gitlab.com/ee/ci/testing/code_quality.html#code-quality-report-format)
for GitLab. This allows GitLab CI jobs to annotate which files need formatting.
Without this, a CI job can only fail, leaving the user to sieve through the
logs to find out which files they need to format.
Copy link

google-cla bot commented Nov 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@octo
Copy link
Contributor Author

octo commented Nov 20, 2024

I'm working on getting the CLA sorted. Luckily there seems to be an existing agreement, so it should just be a matter of getting added to the right group.

@octo
Copy link
Contributor Author

octo commented Nov 20, 2024

I'm working on getting the CLA sorted. Luckily there seems to be an existing agreement, so it should just be a matter of getting added to the right group.

✅ resolved

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Thank you for the feature! This is the exact sort of thing I was hoping the -output_format feature would be used for!

Left two minor comments, but the code overall LGTM.

Would you be able to also add an integration test for this? I haven't written the docs for how to add those yet, this is how you do it:

  • Run TEST_NAME=gitlab_output make command_test_case
  • In `integration_test/command
    • In testdata/gitlab_output, add yaml files to facilitate the test
    • command_test.go add a test function for the gitlab_output folder similar to the other test cases in this file, with the name of the folder and the arguments to supply to the yamlfmt command (in this case -output_format gitlab would be the main thing)
  • Back in the project root, run make integrationtest_update
  • Commit the new changes

internal/gitlab/codequality.go Outdated Show resolved Hide resolved
internal/gitlab/codequality.go Outdated Show resolved Hide resolved
@octo
Copy link
Contributor Author

octo commented Nov 21, 2024

Thank you for the feature! This is the exact sort of thing I was hoping the -output_format feature would be used for!

You're very welcome! Thank you for putting yamlfmt together and being so responsive to PRs!

Would you be able to also add an integration test for this?

✅ done

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

The `slices` package added recently, in Go 1.23.
@octo
Copy link
Contributor Author

octo commented Nov 21, 2024

@braydonk Sorry, I didn't see that go.mod specifies Go 1.18. I've refactored the sorting code to use the sort package instead of slices.

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Sorry I forgot that slices wasn't added until a later version of Go than what yamlfmt tries to be compatible for. (I really should be able to update it since Go 1.18 has been EOL for a while).

Until then, can we change the sorting to the old fashioned way with the sort package? I wrote up some code in a playground that should do the same thing which you're welcome to steal or rewrite. https://go.dev/play/p/S_pGUCRwYan

To fix the lint error, you can run make install_tools addlicense to add the license headers to the new file.

@octo
Copy link
Contributor Author

octo commented Nov 21, 2024

It's already been pushed @braydonk ;)

@braydonk
Copy link
Collaborator

Aha we were working at the same time. Thanks!

@octo
Copy link
Contributor Author

octo commented Nov 21, 2024

Okay, the good news is the technical issues are sorted 🎉

How about the license check? I think I can add the Apache license header without involving legal – it's the same license as the rest of the project after all, so common operating procedure. Not so sure about changing the copyright to "Google LLC" though.

@braydonk
Copy link
Collaborator

Based on reading internal guidance, I think adding just the Apache2 license header with no copyright statements should be fine.

@octo
Copy link
Contributor Author

octo commented Nov 21, 2024

✅ done

make addlicense_check passes locally.

@braydonk
Copy link
Collaborator

Awesome, thanks for working with me on the CI stuff. I appreciate the contribution!

@braydonk braydonk merged commit fab8f29 into google:main Nov 21, 2024
7 checks passed
@octo octo deleted the gitlab branch November 21, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants