-
Notifications
You must be signed in to change notification settings - Fork 50
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
chore: make repo bazel aware & add microgen macro #181
Conversation
I'd like guidance on the macro & workspace naming. I kind of copied the monolith's macros/rules |
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.
In general looks good.
One thing which I did not realize was how much bazel boilerplate code gazelle will generate... It is probably better to keep it like this (since there is special tool to generate it), but it is worth mentioning that it could be a single hand-written BUILD.bazel file for the whole repo (gazelle just tries to be very strict about best practices, generating BUILD.bazel for each package, which makes the boilerplate code "explode"). A hand-written bazel build file will make the build look much less scary =)
WORKSPACE
Outdated
http_archive( | ||
name = "io_bazel_rules_go", | ||
urls = [ | ||
"https://storage.googleapis.com/bazel-mirror/github.com/bazelbuild/rules_go/releases/download/0.19.3/rules_go-0.19.3.tar.gz", |
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.
just curious why there are 2 urls for each? Is this how they recomend it now in rules_go docs?
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.
I copied this from the gazelle repo: https://github.com/bazelbuild/bazel-gazelle#running-gazelle-with-bazel
I can fiddle with removing one if you think its an anti-pattern? LMK
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.
LGTM with a comment.
Also, please make sure that it works with bazel-1.0.0 (released recently).
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.
LGTM (with the tiny string concatenation change we discussed offline)
Fixes #148
gazelle