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

add fmt check in ci #51

Merged
merged 3 commits into from
Dec 23, 2020
Merged

add fmt check in ci #51

merged 3 commits into from
Dec 23, 2020

Conversation

moadibfr
Copy link
Contributor

@moadibfr moadibfr commented Dec 22, 2020

Q A
πŸ› Bug fix? no
πŸš€ New feature? no
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues
❓ Documentation no

Description

We had badly formated (not passed to fmt) file in main. make will create diff that add noise to PR.
I added a check on ci during tests phase to prevent PR to be merge when fmt is not ok.

@moadibfr moadibfr requested a review from a team as a code owner December 22, 2020 16:27
@moadibfr moadibfr force-pushed the fix/fail_on_bad_fmt branch 2 times, most recently from f108ba9 to 1eb97df Compare December 22, 2020 16:36
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #51 (4a3ef87) into main (4d9c048) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #51   +/-   ##
=======================================
  Coverage   66.93%   66.93%           
=======================================
  Files         146      146           
  Lines        3179     3179           
=======================================
  Hits         2128     2128           
  Misses        810      810           
  Partials      241      241           
Impacted Files Coverage Ξ”
pkg/middlewares/aws_instance_eip.go 91.30% <ΓΈ> (ΓΈ)

@@ -17,6 +17,9 @@ jobs:
- image: golang:1.15
steps:
- checkout
- run:
name: Enforce Go Formatted Code
command: test -z "$(go fmt ./...)"
Copy link
Contributor

@wbeuil wbeuil Dec 23, 2020

Choose a reason for hiding this comment

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

Didn't know that you could use test -z to return false or true and that CircleCI would exit. Thanks :)

What I envisioned is more verbose. We could display what are the files we need to re-format just like terraform does.

- run:
      name: Enforce Go Formatted Code
      command: go fmt ./...
- run:
      name: Verify No Code Was Generated
      command: |
            if [[ -z $(git status --porcelain) ]]; then
              echo "Git directory is clean."
            else
              echo "Git directory is dirty. Run `make fmt` locally and commit any formatting fixes or generated code."
              git status --porcelain
              exit 1
            fi

This is what they do in their ci/cd pipeline.

Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

LGTM

@wbeuil wbeuil merged commit 6aa5da8 into main Dec 23, 2020
@wbeuil wbeuil deleted the fix/fail_on_bad_fmt branch December 23, 2020 10:16
@eliecharra eliecharra added the kind/maintenance Refactoring or changes to the workspace label Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/maintenance Refactoring or changes to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants