-
Notifications
You must be signed in to change notification settings - Fork 974
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
Proposal: Try using Earthly instead of Make for one release #5711
Conversation
50360d8
to
8640ba7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
8640ba7
to
15a6d57
Compare
This comment was marked as outdated.
This comment was marked as outdated.
5048c46
to
469458d
Compare
|
||
detect-noop: |
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.
is this intentionally removing the no-op detection or is that also to be reimplemented?
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 did remove it intentionally. My thinking was that when Earthly has a hot cache it'll handle this automatically. It only rebuilds if the code changes, so changing a Markdown file or similar will be a no-op at the Earthly level.
TBD as to whether that will work though. We'd need to setup remote caching for Earthly to be able to handle it automatically.
There's also a handful of relatively long running CI jobs that don't run in Earthly (yet?) - e.g. fuzz tests and CodeQL. We'd need to either get those running in Earthly, or reintroduce detect-noop. I'd personally prefer to get everything running in Earthly if only to keep things simple and consistent.
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.
Thanks @negz for exploring the options here 🙌
I might be one of those who might have invested most to the build submodule, and after some pain and tears, I am feeling relatively comfortable with working with it. However, I can totally see how frustrating it could be for a newcomer. I like the syntax of the Earthfile, especially the fact that it looks like a multi-stage Dockerfile. It look much simpler at the cost of introducing a dependency to an additional tool (different than Make which is available on any system).
I am not sure how it would work when we need to run some docker commands or kind which creates k8s cluster a container since it is already running in a container, but given that you added e2e, I assume it works fine 🤔
Overall, I am fine with experimenting as long as we could have feature parity with the build submodule especially with versioning and artifact publishing / promotion.
42e7be3
to
13603e5
Compare
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
8b47f2d
to
0895b79
Compare
@jbw976 I've spent over a week working only on this PR. So I'm fairly confident it's complete, but I can't guarantee I didn't miss anything. There's a lot of history in our Makefiles, and it's pretty tough to grok what things do and why they do it. I don't think we should let perfect be the enemy of good here though - I expect this PR will need followup work to fix up something or other.
@ytsarev There's a |
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
So that it stops warning about it. Signed-off-by: Nic Cope <nicc@rk0n.org>
The current format uses }, { style, which I find makes it much more laborious to edit, especially when I want to quickly copy and modify a block. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
b82aa03
to
3093e1e
Compare
My instincts are that "out of the cluster" isn't the important part here - the ability to iterate on build/run rapidly is the important part. That |
We could reimplement this with Earthly but I suspect it's not used. Signed-off-by: Nic Cope <nicc@rk0n.org>
I'm not currently reimplementing the crds.clean target that trims the leading --- from the CRDs because it's unclear whether it's still needed. Signed-off-by: Nic Cope <nicc@rk0n.org>
# local environment, not a container. The kind cluster will keep running until | ||
# you run the unhack target. Run hack again to rebuild Crossplane and restart | ||
# the kind cluster with the new build. | ||
hack: |
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.
the hack
command ran into an error for me, it looks like unhack
was executed and could not execute the kind
binary 🤔
❯ earthly +hack
Init 🚀
————————————————————————————————————————————————————————————————————————————————
buildkitd | Found buildkit daemon as docker container (earthly-buildkitd)
Build 🔧
————————————————————————————————————————————————————————————————————————————————
logbus | Setting organization "crossplane" and project "crossplane"
+hack | --> FROM +base
+hack | darwin/arm64
+hack | --> BUILD +unhack
+unhack | --> FROM +base
+kind-setup | --> FROM +base
c/curl:8.8.0 | --> Load metadata curlimages/curl:8.8.0 linux/arm64
Warning: you are not logged into registry-1.docker.io, you may experience rate-limitting when pulling images
+kind-setup | --> FROM curlimages/curl:8.8.0
+kind-setup | [----------] 100% FROM curlimages/curl:8.8.0
+kind-setup | --> RUN curl -fsSLo kind https://github.com/kubernetes-sigs/kind/releases/download/${KIND_VERSION}/kind-${NATIVEOS}-${NATIVEARCH}&&chmod +x kind
+unhack | darwin/arm64
+unhack | --> COPY +kind-setup/kind .hack/kind
+unhack | darwin/arm64
+unhack | --> RUN .hack/kind delete cluster --name crossplane-hack
+unhack | /bin/sh: .hack/kind: cannot execute binary file
+unhack | ERROR Earthfile:95:2
+unhack | The command
+unhack | RUN .hack/kind delete cluster --name crossplane-hack
+unhack | did not complete successfully. Exit code 126
================================== ❌ FAILURE ===================================
+unhack *failed* | Repeating the failure error...
+unhack *failed* | darwin/arm64
+unhack *failed* | --> RUN .hack/kind delete cluster --name crossplane-hack
+unhack *failed* | /bin/sh: .hack/kind: cannot execute binary file
+unhack *failed* | ERROR Earthfile:95:2
+unhack *failed* | The command
+unhack *failed* | RUN .hack/kind delete cluster --name crossplane-hack
+unhack *failed* | did not complete successfully. Exit code 126
+hack | earthfile2llb for +unhack: Earthfile:95:2 apply RUN: unlazy force execution: error calling LocalhostExec: exit code: 126
+hack | in +unhack
🛰️ Reuse cache between CI runs with Earthly Satellites! 2-20X faster than without cache. Generous free tier https://cloud.earthly.dev
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 think the problem was that TARGETOS
binaries won't work for LOCALLY
targets, since TARGETOS
will always be Linux. I've pushed a commit that I hope will fix this. Verified it still works when run from Linux.
This is hacky and I hate it, but I can't think of a better approach. Signed-off-by: Nic Cope <nicc@rk0n.org>
|
||
generate.done: crds.clean crds.patch | ||
|
||
gen-kustomize-crds: |
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've intentionally skipped reimplementing this target - my hunch is it's not used. We can add it if folks really need it, but I'm not sure how valuable it is.
Its version isn't specified there anymore. Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
For me, the iteration process is incredibly quick. I can run the crossplane core process locally in a terminal tab, receive immediate feedback, make changes, restart the process, and repeat. I can also use advanced local debugging tools like Delve if needed. This method is significantly faster than the |
| Run linters | ~3 minutes | ~4 minutes | | ||
| Run unit tests | ~3 minutes | ~2.5 minutes | | ||
| Publish artifacts | ~12 minutes | ~14 minutes | | ||
| Run E2E tests | ~12 minutes | ~14 minutes | |
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.
Curious @negz if you got a sense for any impact on reliability and/or performance of running e2e tests locally. I just thought of this now while running earthly -P +e2e
(which I believe runs -test-suite=base
). It took about 20 mins to run that suite and it had 10 failures:
DONE 114 tests, 13 skipped, 10 failures in 1196.054s
I'm not sure if that's slower or less reliable - have you developed a sense for that while testing yourself? 🤔
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.
No idea.
That said, I can't see why it would be slower or less reliable. At the end of the day it's the same code running in a Docker container spun up using kind
whether that was done by Make or Earthly.
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.
FWIW I just ran earthly -P +e2e
locally and everything passed. I'm not sure how long it took though - I don't get the DONE
summary for whatever reason.
TARGETPLATFORM is incorrect on MacOS, where TARGETOS will be Linux. There we want USERPLATFORM, i.e. Darwin. Signed-off-by: Nic Cope <nicc@rk0n.org>
4eb5645
to
ba9e43f
Compare
Description of your changes
This PR adds a one-pager proposing we adopt https://earthly.dev. Earthly would replace the build submodule.
I propose we try Earthly in
crossplane/crossplane
andcrossplane/crossplane-runtime
, for one release. After releasing Crossplane v1.17 we'd assess whether we wanted to commit to Earthly, revert to the build submodule, or something else. I don't think we should attempt to migrate providers until we've lived with Earthly for a while here.The PR also contains a proof-of-concept port of most of this repository's
Makefile
to an EarthlyEarthfile
. I believe the only thing missing is the rules that publish binaries and Helm charts to S3. Take a look at theEarthfile
and the GitHub Actions output to get a sense of what working with Earthly is like.I have:
Runearthly +reviewable
to ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.y
labels to auto-backport this PR.Need help with this checklist? See the cheat sheet.