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

Fix fmt in ci #351

Merged
merged 5 commits into from
Oct 26, 2020
Merged

Fix fmt in ci #351

merged 5 commits into from
Oct 26, 2020

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Oct 26, 2020

Description

Ensures all our code is go fmt (really goimports clean) by checking in CI and failing if there's a change.

Why is this needed

Fixes: #350

How Has This Been Tested?

goimports is happy and tests aren't affected.

How are existing users impacted? What migration steps/scripts do we need?

No impact.

mmlb added 2 commits October 26, 2020 16:21
To pick up goimports tool.

Signed-off-by: Manuel Mendez <[email protected]>
Wasn't caught by CI when introduced.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb mentioned this pull request Oct 26, 2020
3 tasks
@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #351 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #351   +/-   ##
=======================================
  Coverage   23.79%   23.79%           
=======================================
  Files          14       14           
  Lines        1244     1244           
=======================================
  Hits          296      296           
  Misses        928      928           
  Partials       20       20           
Impacted Files Coverage Δ
http-server/http_handlers.go 6.68% <ø> (ø)
http-server/http_server.go 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2683ed2...036e3a5. Read the comment docs.

@mmlb mmlb requested a review from kqdeng October 26, 2020 20:22
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Oct 26, 2020
mmlb added 3 commits October 26, 2020 16:34
Imports should be grouped by either in stdlib or not as goimports does
by default. goimports won't ungroup groups its comes across though, but
there's no reason for us to have them in our project.

Fixed with:

sed -i '/^import (/,/^)/ { /^\s*$/ d}' **/*.go
goimports -w .

Signed-off-by: Manuel Mendez <[email protected]>
goimports is a superset of go fmt, it also ensures the imports are
grouped and sorted nicely. Most importantly it has a `-d`/diff mode that
we can use to test for changes more easily than what would be needed if
we stayed with `go fmt`.

Signed-off-by: Manuel Mendez <[email protected]>
Fail CI if goimports has any changes, otherwise our intent of having `go
fmt`'d code isn't actually guaranteed.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb merged commit 9e7d539 into tinkerbell:master Oct 26, 2020
@mmlb mmlb deleted the fix-fmt-in-ci branch October 26, 2020 21:22
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
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.

Go fmt output isn't checked in CI
2 participants