Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Run golangci-lint as part of pull request workflow #604 #607

Merged
merged 1 commit into from
May 15, 2020
Merged

Run golangci-lint as part of pull request workflow #604 #607

merged 1 commit into from
May 15, 2020

Conversation

abstractj
Copy link

@abstractj abstractj commented May 7, 2020 •
edited
Loading

Changes

  • Merge build and linter jobs in a single file
  • Runs tests for go 1.13 and 1.14
  • Introduces code coverage with coveralls
  • Fixes some issues reported by golangci-lint and relax/disable some
    rules until the codebase is refactored

Running example:

Testing this PR

  • Download the build.yml file from this PR and add it to .github/workflows into the fork of
    Louketo
  • Push the changes to master at your fork
  • Submit a pull request to your own fork and GH action should be triggered

Relates to #615
Resolves #604

JoelSpeed
JoelSpeed reviewed May 8, 2020
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Is there an easy way we can test these workflows before merging?

JoelSpeed
JoelSpeed reviewed May 8, 2020
View reviewed changes
.github/workflows/go.yml Outdated
Comment on lines 28 to 31
if [ -f Gopkg.toml ]; then
curl https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
dep ensure
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

This repo uses go modules right? we can drop this

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @JoelSpeed updated. I just enabled it on GH and missed that part.

@abstractj
Copy link
Author

abstractj commented May 8, 2020 •
edited
Loading

Is there an easy way we can test these workflows before merging?

Yes, you can look at the fork here https://github.com/abstractj/louketo-proxy/actions. As you may notice, there are several alerts there for golangci-lint. Which ones do you think we should disable?

IMO the alerts for whitespace make sense, but not sure about the complexity warnings. Because sometimes we only updated the code to please the Linter. But if you consider this something important, we can keep it.

@abstractj abstractj requested a review from JoelSpeed May 8, 2020 14:54
@JoelSpeed
Copy link
Contributor

In general I think the complexity ones are good, but you can get ones that are a bit annoying, such as the ones highlighted here. I believe we can add an exception to .golangci.yaml if we want to keep it turned on. I have no strong opinion either way on this one

@abstractj
Copy link
Author

@JoelSpeed np, let's keep it. Found a typo on my YAML file, but it's fixed now. If there's anything else that you would like to change, please let me know.

@JoelSpeed
Copy link
Contributor

Do you think we should try and fixup the linting issues before merging this? Probably as a separate PR?

@abstractj
Copy link
Author

@JoelSpeed of course, we can do that, there's no need to rush about merging this.

@abstractj abstractj assigned stianst and unassigned JoelSpeed May 14, 2020
Run golangci-lint as part of pull request workflow louketo#604
92a2009 
- Merge build and linter jobs in a single file
- Runs tests for go 1.13 and 1.14
- Introduces code coverage with coveralls
- Fixes some issues reported by golangci-lint and relax/disable some
rules until the codebase is refactored

Resolves louketo#604
@abstractj
Copy link
Author

@stianst could you please review?

@abstractj abstractj merged commit 8e8510a into louketo:master May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stianst stianst Awaiting requested review from stianst

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

Assignees

@stianst stianst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run golangci-lint as part of pull request workflow
3 participants
@abstractj @JoelSpeed @stianst