-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
build: replace kit
with task
#11928
base: main
Are you sure you want to change the base?
build: replace kit
with task
#11928
Conversation
- Task is a very popular, well-maintained, Go-based task runner with a nearly identical spec to `kit`, as can be seen in the diff - basically can do virtually all of the same things and a good number of other things, with lots of docs and active maintenances - intentionally made this commit have as few changes as possible to reduce the diff and make it look very familiar - used one-line arrays instead of multi-line to be similar to `kit`'s `make`-like task dep list - used singular `cmd` syntax instead of the usual plural `cmds` syntax Signed-off-by: Anton Gilgur <[email protected]>
- required name change - convention to use capital for the first letter: https://taskfile.dev/styleguide/#use-taskfileyml-and-not-taskfileyml Signed-off-by: Anton Gilgur <[email protected]>
- new lines between tasks: https://taskfile.dev/styleguide/#add-spaces-between-tasks - consistent ordering of keywords: https://taskfile.dev/styleguide/#use-the-correct-order-of-keywords - strangely it doesn't mention keywords _within_ tasks, but I did `cmd` -> `dir` -> `deps` -> `sources` -> `env` consistently - similar to how I ordered the GH Actions keywords too Signed-off-by: Anton Gilgur <[email protected]>
tasks: | ||
go-deps: | ||
cmd: go mod download | ||
# mutex: download |
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 don't think any of these mutexes are actually strictly necessary, two downloads and two builds can run in parallel, they just may bottleneck (or they may not, I don't believe they do on my laptop), so it might be a concurrency optimization to run them serially instead.
Task does have the run
key to ensure something runs only once. And it does have a serial syntax, but it's not as flexible as a string for a mutex lock (not entirely the same semantics as a mutex does not specify ordering)
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.
Removed the download
mutex in #13350
@@ -0,0 +1,109 @@ | |||
# yaml-language-server: $schema: https://taskfile.dev/schema.json |
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 schema is a pretty nice addition too 😉
it's got integrations with VSCode and other editors as well if that's your preference
All of the E2E tests seem to be failing on port issues (like the ones I had with |
.PHONY: task | ||
task: | ||
# update this in Nix when upgrading it here | ||
ifneq ($(USE_NIX), true) | ||
npm i -g @go-task/[email protected] |
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.
Needs updating in the Nix file, happy to do that myself if you want?
Technically the Nix setup doesn't need task though because it already has something similar.
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.
@isubasinghe could you elaborate on this? You mentioned devenv
before, but I didn't know that had task runner support (again, my Nix knowledge is pretty dated)?
We discussed it at the Contributor meeting too if it could be used as a replacement on all environments, but no one knew the answer so we were looking to you for more info.
There was also this warning in the Nix docs that we weren't sure exactly what it applied to
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.
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.
Yeah it does have a task runner, infact Nix can replace the Makefile and task/goreman/etc.
https://github.com/argoproj/argo-workflows/blob/master/dev/nix/flake.nix#L393-L403 is how the processes to run are declared.
There is a manual step of setting up the k8s cluster needed at the moment but even this can be fully integrated into Nix.
That controller should have a version attached to it, I did not set this version. So production builds using Nix are not really possible yet. This is still very fixable though, more than happy to talk about it if this is something we want to do.
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.
Yeah it does have a task runner, infact Nix can replace the Makefile and task/goreman/etc.
https://github.com/argoproj/argo-workflows/blob/master/dev/nix/flake.nix#L393-L403 is how the processes to run are declared.
Yea I've seen that -- that's equivalent to goreman
but that doesn't have deps
, sources
, env
etc -- features of task
, kit
, and make
. Unless that feature exists somewhere too and just isn't being used?
Generally looks good to me, not approving or disapproving yet, will wait till tests pass. |
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.
Per our discussions at today's contributors meeting, one better option is to revert back to use goreman which is widely adopted and used by Argo CD.
@terrytangyuan There will almost certainly be conflicts. |
I don't think there was consensus on "better", just that Workflows, CD, and Akuity already use(d) it, so it would be more familiar within Argoproj. We had consensus on removing
If that's what we want to do, I can tackle that |
- following the installation guide: https://taskfile.dev/installation/ - use `npm` for the `Makefile` install as it's compatible on different OSes and matches some of our existing tools (like `mdspell`, `markdownlint`, etc) - NOTE: this does still need adding to the Nix dir - replace `kit` in `Makefile`, `pre-build.sh`, and `ci-build.yaml` with `task` Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
- it was removed when `goreman` was replaced by `kit` in 05e1318 - add it back since `task` does not do port-forwarding - should hopefully fix all the CI issues in E2E tests Signed-off-by: Anton Gilgur <[email protected]>
36d9e60
to
bf07cea
Compare
- name: Start port forward | ||
run: ./hack/port-forward.sh |
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 was removed in the goreman
-> kit
migration (this line), added it back now since task
does not handle port forwarding
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 fixed the Controller E2E tests. some Server tests are failing because apparently the Server didn't start at all. apparently task
was waiting for deps
to finish (which totally makes sense), but starting a servers will never finish. so the cmd
s need to be backgrounded properly. then everything works ok.
I think kit
's ports
keyword (mentioned above) was just watching for ports to be opened before proceeding to the next task. nifty feature for servers.
- i.e. `controller`, `server`, and `ui`, which all just run forever - previously `task` was waiting for these to complete as `deps`, but that would never happen because they run forever Signed-off-by: Anton Gilgur <[email protected]>
- it was running all the time before in `kit` and now `task`, but it should only run when there are dep changes Signed-off-by: Anton Gilgur <[email protected]>
I have good news and bad news. Good news:
Bad news:
|
Let’s correct for the record:
@juliev0 do you need to open a bug? Why does it need to be changed? I don’t see any discussion of the problem being solved here. That’s said, simpler toolchains are cheaper to maintain. If Nix can do the job , can we try that? |
Sure, I’ll open a bug. Thanks Update: added here |
@alexec I'm not sure the trade offs of the various tools (need to understand what @agilgur5 lists above). I think one issue if we end up keeping it, is that it needs more documentation on how users should use it in the "running locally" page. I think people are generally confused about where to find their log files, they don't know anything about the tasks.yaml file, etc. Also, need to make sure all of the core functionality works. Apologies for not writing the bug as an issue earlier. |
It looks like we've had some tool creep in workflows (CD may have had the same) and this has resulted in too many tools. Kit prints logs to the console if you run |
To be fair, I didn't say it wasn't. I merely compared its activity to
All I know is that when I swapped it out, my fans made a lot less noise and my memory usage was considerably lower 😅 .
I listed out related discussions in issues and Slack in my opening comment, but Julie has succinctly summarized them above; a handful of bugs and a lack of familiarity and documentation with
There's a separate discussion on Nix above. It can fulfill what @alexec while you are here, would you mind elaborating on the reasons for using |
I've raised a bug. but more diagnostic info would be useful. Your screen shot indicates that a container is using a lot of memory, but does not say what that container is? Do you have a
I count 2 bugs?
This is a reasonable argument. This would make tools like Nix and Goreman more attractive.
Kit is not a replacement for me Make. It is a replacement for foreman. |
It's the Workflows
That is enough for a handful, to be fair 😝. EDIT: filed kitproj/kit#44 regarding Codespaces. I literally only tried it myself yesterday
That's the primary reason, to be fair 👍
🤔 It does a lot more than |
talked a bit with Alex offline over CNCF Slack. we didn't get into the full meat of this yet but I think Alex specifically meant basically a lot of tools with partial overlap |
Should we live with it if |
Should we close this? |
Thanks for following up on this Terry.
I do still have a few months-old open issues on Kit, so it is not quite all clear skies. In particular, kitproj/kit#48 is one I hit into with some frequency (and can take a few minutes to realize). The workaround is simple (quit Since I had initially thought to do a partial move to I think reducing the scope of
Otherwise I think we can close this since |
What about Tilt.dev, or DevSpace.sh? |
It's been a long time since I've played around with Tilt (and I don't think I ever gave Devspace a spin as it's quite similar) but it wasn't quite clear if it supports background tasks (i.e. servers) and host processes (i.e. running binaries or |
Motivation
Task is a very popular (~9k stars), well-maintained (has releases at least once a month in the past 6 months), well-documented, Go-based task runner with a nearly identical spec to
kit
, as can be seen in the diffComparatively,
kit
was only created in the past year by Alex, is not well-known (~40 stars, and anecdotally, I'd never heard of it before), and seems to be sporadically maintainedTo be explicit, I'm not big on Task or anything (Earthly is my current go-to, as mentioned on Slack etc) -- it was just the most similar and most popular alternative I knew of
Modifications
intentionally made this PR have as few changes as possible to reduce the diff and make it look very familiar
kit
'smake
-like "stringy types"cmd
syntax instead of the usual pluralcmds
syntaxthen followed Task's styleguide:
Taskfile.yaml
: https://taskfile.dev/styleguide/#use-taskfileyml-and-not-taskfileymlcmd
->dir
->deps
->sources
->env
consistentlyreplace all installation and usage of
kit
withtask
npm
for theMakefile
install as it's compatible on different OSes and matches some of our existing tools (likemdspell
,markdownlint
, etc)kit
inMakefile
,pre-build.sh
, andci-build.yaml
withtask
Verification
This is a draft, I actually haven't tested it yet