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

Figure out what to do with bazel #1015

Closed
chuckha opened this issue Aug 14, 2019 · 10 comments · Fixed by #1076
Closed

Figure out what to do with bazel #1015

chuckha opened this issue Aug 14, 2019 · 10 comments · Fixed by #1076
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@chuckha
Copy link
Contributor

chuckha commented Aug 14, 2019

/kind feature

Describe the solution you'd like
It feels like we are moving away from bazel for many things. We no longer need to generate YAML like we used to and the go toolchain seems to be satisfactory for other cluster-* providers.

Should we remove all of bazel? some of bazel?

/priority important-longterm
/milestone Next

@k8s-ci-robot k8s-ci-robot added this to the Next milestone Aug 14, 2019
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Aug 14, 2019
@chuckha
Copy link
Contributor Author

chuckha commented Aug 14, 2019

We have also defined kustomize rules in bazel as well as image building which we should absolutely reduce to one way to do it.

@randomvariable
Copy link
Member

Think we need to be all in on Bazel so we get the full benefits of caching, or not use it at all.

@vincepri
Copy link
Member

vincepri commented Aug 14, 2019

We should remove Bazel as much as we can. It's useful for things like aligning on CI tooling versions (like golangci-lint). If we can get rid of that as well, we can remove all of it.

@chuckha
Copy link
Contributor Author

chuckha commented Aug 14, 2019

@randomvariable is there some centralized k8s cache we can use or is this bring your own cache sorta deal?

@randomvariable
Copy link
Member

@chuckha There's a centralised k8s cache for PR tests, at least during the v1alpha1 time, it would have reduced our build times from 10 mins to 2mins with the integration tests.

I'm not sure if caches actually work well over the WAN, depending on how Bazel chunks cache objects - large numbers of small objects could possibly take longer to download than compile locally.

There are some areas that we hadn't fully Bazelised, so the cache didn't operate fully - go deps and things that were reliant on go generate that hadn't been transformed into Bazel equivalents (things that generated the zz_deepcopy.gos came to mind).

We also had a bunch of scripts that would generate files, mostly YAML that you would then commit to the repo that would then get editors to behave correctly. Bazel isn't really designed around that model, and we had bad hacks using the Makefile to get the behaviour we wanted, but there's possibly some sandbox options that could do the same thing. Think I had some experiments in #371, but was afraid to commit them in the end.

Finally, I think there was some work to be done around getting bazel test to run more efficiently.

@timothysc
Copy link
Member

timothysc commented Aug 14, 2019

what's the overall benefit that we get here? If it's just caching I think we can rely on moore's law, and the code base is really not that large in comparison to k/k. Which I can compile from 0 in under 2 minutes on my machine now a day.

@vincepri
Copy link
Member

I'd also add that we've seen decreased build time switching to Go Modules (which also removed Bazel). If there was a benefit, it seems we're definitely past it 🙂

@detiber
Copy link
Member

detiber commented Aug 15, 2019

So, some of the reasons that drove us to bazel originally:

  • Alternating PRs updating the go fmting of code based on client go versions being different between contributors.
  • Different versions of client tooling (kustomize, kubectl, etc) causing different behaviors and hard to replicate issues between contributors
  • Differences between contributor local environments causing issues, such as different behaviors between gnu sed and bsd sed, is envsubst available, different shells affecting test tooling

That said with bazel came:

  • more cognitive overhead required to add and debug tests
  • more general overhead in running tests
  • more maintenance time for updating to keep working with newer versions of bazel

I like the idea of removing bazel as much as possible, but I also don't want to revert to the previous state where everyone was running different versions of things and we were spending a ton of time debugging why one individual is seeing test failures that others aren't or why the CI environment is showing errors that are not showing locally.

@chuckha
Copy link
Contributor Author

chuckha commented Aug 16, 2019

for certain tools we can leverage the tools.go + go.mod + // +build tags to enforce versions of dependencies, but that would require everything to go through a makefile (or somewhere that would enforce which tool to use).

An alternative to the versions problem is to have a script that prints out the versions of your current environment. Basically a shell script that says "are you using the right version of kustomize, are you using the right version of ..." . Either it could simply print out which version you're using (helpful for filing issues/getting help) or it could tell you what versions are not expected...

@vincepri
Copy link
Member

I'm fine picking the right tool for the right job. Bazel has been working fine in CAPI for linting purposes, so I'm +1 keeping just for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants