Skip to content

Conversation

@hiuns
Copy link
Contributor

@hiuns hiuns commented Dec 20, 2024

This PR solves #17

Add an Action to enforce Go code formatting using gofmt and block unformatted code. This PR intentionally excludes lint since it requires external dependencies. It also intentionally doesn't auto-commit formatted code on push, as preferences vary.

Changes

  • Workflow: lint.yaml checks formatting using gofmt. Named "lint.yaml" instead of "check-format.yaml" because it's meant to be extended in the future.

Future Improvements

  1. Consider other lint options such as line length, typo, and case-consistency.
  2. Consider auto-committing format/lint fixes after making a PR.

Usage

  • Locally:
    • Run bazelisk run @go_sdk//:bin/gofmt -- -w .
  • CI/CD:
    • Runs on pull_request.

@hiuns hiuns changed the title Add GitHub workflow for Go formatting Add GitHub workflow for auto formatting Dec 20, 2024
@hiuns hiuns changed the title Add GitHub workflow for auto formatting Add GitHub workflow for checking code format Dec 20, 2024
@dayeol
Copy link
Contributor

dayeol commented Dec 20, 2024

Hi @hiunshim!

Thank you for the PR!

We use Bazel instead of Make for our build system.
In Bazel, we already import rules_go, which includes the SDK with gofmt binary in it.
Thus, you can run the following command to invoke gofmt without installing anything

bazelisk run @go_sdk//:bin/gofmt -- -w .

Could you please change it to use the bazel command, instead of make?
Also, I think CI/CD needs to fail if gofmt changes any file after running it.

@hiuns
Copy link
Contributor Author

hiuns commented Dec 21, 2024

Hi @dayeol!

Thank you for reviewing the PR and giving me detailed explanation.
I updated it to use Bazel instead of Make as requested.

I considered two options:

  1. format all go files
  2. format only staged files (through git diff)

I think option 1 is sufficient at the moment so I chose that. I read that Github Actions runners are ephemeral so no files will persist after the job.

I'll also link option 2 code snippet for future reference. Option 2 has been tested separately as well, but the overhead was unclear compared to option 1.

Tests
CI/CD Passing Test: https://github.com/hiunshim/manatee/actions/runs/12444917699
CI/CD Breaking Test: https://github.com/hiunshim/manatee/actions/runs/12444919510

Tests Screenshot
Screenshot 2024-12-21 at 4 13 19 AM

@dayeol
Copy link
Contributor

dayeol commented Dec 29, 2024

@hiunshim Thank you! This PR LGTM.

@hiuns
Copy link
Contributor Author

hiuns commented Dec 29, 2024

@dayeol Awesome! Could you please merge it? It says "Merging is blocked - Merge is not an allowed merge method in this repository. Only those with write access to this repository can merge pull requests."

@dayeol dayeol merged commit c4e1a5e into manatee-project:main Dec 31, 2024
2 checks passed
@hiuns hiuns deleted the chore/17-init-cicd-lint branch December 31, 2024 05:49
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