-
-
Notifications
You must be signed in to change notification settings - Fork 219
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 magefile for running development commands #315
Conversation
*.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was previously formatted with CRLF
magefiles/magefile.go
Outdated
return errCommitFormatting | ||
} | ||
|
||
return sh.RunV("go", "run", fmt.Sprintf("github.com/golangci/golangci-lint/cmd/golangci-lint@%s", golangCILintVer), "run") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use go run
for commands since it ensures the correct version is used no matter what's up with the user's environment and removes the need for any installation
It looks like a nice idea, but what happens with the documentation? We would be exporting development code and unnecessary documentation to pkg.go.dev ? |
@jptosso Currently it seems to render as an empty package on local godoc. I think If going in on magefile, then this could be improved later by moving them into a separate repo and importing them here and other corazawaf repos like coraza-caddy https://magefile.org/importing/. How does that sound? |
@jptosso Just wondering if you have any thoughts on ^^. Ran into a bit of trickiness with failing lint recently due to the lack of command so would be nice to have something in. Since we have a few repositories in this org, I think sharing the logic will be a huge win for us. If you'd rather start with that instead of a magefiles directory in this repo, then I could do that if I have a magefiles repo :) |
I like the idea, but I have never seen it within the code. Why couldn't we use Makefile? or package.json Adding a new package might create confusion. In my case, VSCode takes care of everything, and pre-commit takes care of validations. |
Makefile and npm require separate tools from our required one, Go. This is particularly problematic on Windows where neither is that easy to install, but also does require steps on Linux. Java shops use Java for tooling, npm shops use node, I do agree with the vision that Go shops should just use Go since it works pretty well from what I've found. Of course this is all quite fuzzy so I think it just needs a decision of any form. If Makefile seems better in the end, happy to go with it. But for sure make is definitely the worst of all those options in terms of sharing among multiple repos :) For reference if it helps, Hugo is a famous project using mage. https://github.com/gohugoio/hugo/blob/master/magefile.go |
Ok, that makes sense, we could also add the option to test modsecurity and CRS. But could we keep it in the same magefile.go so we don't generate another package? |
Thanks @jptosso - reworked to not use a separate package, confirmed there is no influence on godoc now |
Nice work, I think we should run this same commands on CI. |
@jcchavezs Yup - thinking of looking into it after #334 and backporting these to v2 |
This is an example for #313 using mage. Happy to switch to make if people prefer it. I like mage myself since it only requires Go, like the actual build itself - this is particularly useful on Windows, though also can help when setting up scripts run in slim docker containers.
Note: that go.mod and go.sum can only be modified for tested dependency updates or justified new features.
Make sure that you've checked the boxes below before you submit PR:
Thanks for your PR ❤️