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

build: replace kit with task #11928

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Oct 2, 2023

Motivation

To 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

Verification

This is a draft, I actually haven't tested it yet

- 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 <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/build Build or GithubAction/CI issues label Oct 2, 2023
tasks:
go-deps:
cmd: go mod download
# mutex: download
Copy link
Member Author

@agilgur5 agilgur5 Oct 2, 2023

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)

Taskfile.yml Show resolved Hide resolved
@@ -0,0 +1,109 @@
# yaml-language-server: $schema: https://taskfile.dev/schema.json
Copy link
Member Author

@agilgur5 agilgur5 Oct 2, 2023

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

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 2, 2023

So far task also seems much more efficient than kit. It's running pretty silently on my laptop in the background whereas previously make start UI=true would make all my fans turn on quite loudly.

Screenshot from Docker Desktop (where you can also see how much resources I allocated):
Screenshot 2023-10-02 at 4 21 21 PM

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 2, 2023

All of the E2E tests seem to be failing on port issues (like the ones I had with devcontainer CLI above). CI doesn't run in a devcontainer though, so those should be a bit different

@terrytangyuan
Copy link
Member

cc @isubasinghe @alexec

Comment on lines +520 to +524
.PHONY: task
task:
# update this in Nix when upgrading it here
ifneq ($(USE_NIX), true)
npm i -g @go-task/cli@3.30.1
Copy link
Member

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.

Copy link
Member Author

@agilgur5 agilgur5 Oct 3, 2023

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked it myself and it looks like devenv up is a replacement for goreman, but neither are full task runners, they just run processes concurrently (no deps, sources, env, etc).

task and kit are make replacements rather than goreman replacements.

Copy link
Member

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.

Copy link
Member Author

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?

@isubasinghe
Copy link
Member

Generally looks good to me, not approving or disapproving yet, will wait till tests pass.

Copy link
Member

@terrytangyuan terrytangyuan left a 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.

@isubasinghe
Copy link
Member

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
This might be the easiest way to get rid of kit, a careful revert should do the trick.

There will almost certainly be conflicts.

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 3, 2023

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.

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 kit, but there were a few options on the table, such as task, goreman, Nix devenv (see above), and just sticking to make (which can also run concurrent tasks and already has task dependencies etc).

@terrytangyuan This might be the easiest way to get rid of kit, a careful revert should do the trick.

There will almost certainly be conflicts.

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 <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
Comment on lines +156 to +157
- name: Start port forward
run: ./hack/port-forward.sh
Copy link
Member Author

@agilgur5 agilgur5 Oct 3, 2023

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

Copy link
Member Author

@agilgur5 agilgur5 Oct 3, 2023

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 cmds 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 <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
@agilgur5
Copy link
Member Author

agilgur5 commented Oct 4, 2023

I have good news and bad news.

Good news:

  • I have figured out how to get the tests to pass (needs some more modifications though)

Bad news:

@alexec
Copy link
Contributor

alexec commented Oct 9, 2023

Let’s correct for the record:

  • Kit is maintained, but it’s basically feature complete. A lack of activity is a good thing, look at ‘jq’.
  • That said, the last release was 3 weeks ago.
  • Kit has no open bugs.
  • Kit is probably not the cause of the fan issue you mention. I’ve heard no similar reports from other users.

@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?

@juliev0
Copy link
Contributor

juliev0 commented Oct 9, 2023

Let’s correct for the record:

  • Kit is maintained, but it’s basically feature complete. A lack of activity is a good thing, look at ‘jq’.
  • That said, the last release was 3 weeks ago.
  • Kit has no open bugs.
  • Kit is probably not the cause of the fan issue you mention. I’ve heard no similar reports from other users.

@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

@juliev0
Copy link
Contributor

juliev0 commented Oct 9, 2023

@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.

@alexec
Copy link
Contributor

alexec commented Oct 10, 2023

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 CI=1 kit up, that might make it easier to debug by default.

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 10, 2023

  • Kit is maintained, but it’s basically feature complete. A lack of activity is a good thing, look at ‘jq’.

To be fair, I didn't say it wasn't. I merely compared its activity to task (which is no doubt more active). Discussed more in kitproj/kit#36 (comment).
Also I'm not sure that jq is a fair comparison, given that it's much smaller in scope. kit's README also compares to jq as a single binary, but jq is ~270kb while kit is ~8mb. But let's keep that in the other issue.

Kit is probably not the cause of the fan issue you mention. I’ve heard no similar reports from other users.

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 did not trace it carefully, but it seemed like there might be a memory leak in the auto-restart feature or something as I noticed the memory just kept increasing when I made changes to the code and not going back down. It could be a memory leak in one of the tasks themselves too though; again, I did not trace it, that was merely an observation and not one of the initial motivators.

Why does it need to be changed? I don’t see any discussion of the problem being solved here.

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 kit.

That’s said, simpler toolchains are cheaper to maintain. If Nix can do the job , can we try that?

There's a separate discussion on Nix above. It can fulfill what goreman did, but it does not do all that kit does.

@alexec while you are here, would you mind elaborating on the reasons for using kit inside of Workflows? It replaced goreman, but kit is more generally a make replacement (with process management as an additional feature), yet make was not fully replaced and still makes up the bulk of the scripting. If make were wholly replaced, I'd understand that (I am certainly not the biggest make fan), but as only a partial replacement I was a bit confused. goreman is similarly a very tiny-scoped tool that does pretty much one thing

@alexec
Copy link
Contributor

alexec commented Oct 10, 2023

All I know is that when I swapped it out,

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 ps output that shows that it is kit?

handful of bugs

I count 2 bugs?

lack of familiarity

This is a reasonable argument. This would make tools like Nix and Goreman more attractive.

kit is more generally a make replacement for Make

Kit is not a replacement for me Make. It is a replacement for foreman.

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 10, 2023

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 ps output that shows that it is kit?

It's the Workflows devcontainer. I can put more details in the relevant bug, but as I mentioned before, it was just an observation, so I hadn't looked into it in detail to check for a root cause.

I count 2 bugs?

That is enough for a handful, to be fair 😝.
There are some others like Codespaces running indefinitely on kit pre-up: #11423. I even got a notification from GH that it used up 24 hours of compute before I even had access to a terminal (it apparently ran all night even after I closed the tab and turned off my computer).
I just looked into that yesterday and think it can be solved by switching the devcontainer's onCreateCommand to a postCreateCommand, but something weird is going on with how Codespaces is interacting with kit specifically at build-time. I saw it restart a few processes repeatedly too, not sure why.
I wouldn't be surprised if there were other bugs too.

EDIT: filed kitproj/kit#44 regarding Codespaces. I literally only tried it myself yesterday

This is a reasonable argument. This would make tools like Nix and Goreman more attractive.

That's the primary reason, to be fair 👍
It just pops up occasionally from a bug or contributor issue. I wrote this PR as I was literally just comparing task's syntax and wanted to see how easy it would be to transition. It was pretty easy, though task doesn't do process management like foreman / goreman unfortunately 😕

Kit is not a replacement for me Make. It is a replacement for foreman.

🤔 It does a lot more than foreman though? As it's a general task runner (like task and make) rather than a simple process manager

@agilgur5
Copy link
Member Author

agilgur5 commented Oct 10, 2023

🤔 It does a lot more than foreman though? As it's a general task runner (like task and make) rather than a simple process manager

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 make as a "build tool" rather make as a "task runner".
but we mostly use it as a "task runner" in Workflows and that is the primary comparison to task as well (and task compares itself to make very explicitly).
(Earthly is more of a "build tool", for comparison)
Nix is more of a "dependency management tool", so also a bit orthogonal 😅

basically a lot of tools with partial overlap

@terrytangyuan
Copy link
Member

Should we live with it if kit already has all the features we need and there are no known stressing bugs?

@terrytangyuan
Copy link
Member

Should we close this?

@agilgur5
Copy link
Member Author

Thanks for following up on this Terry.

and there are no known stressing bugs?

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 kit and run yarn install outside of kit, then restart kit), but not a great experience and could be especially confusing for new contributors.

Since task still does not handle background processes and contributors preferred kit's DX to goreman's DX, I was thinking what we should do is reduce kit to a background process runner and move the rest of the tasks out to limit the scope and bugs like the above.

I had initially thought to do a partial move to task to complete that, but at that point I was wondering if we should just entirely replace Make with another tool (task or otherwise). I thought that some contributors may not want to move away from Make, especially since we've been using it for so long and have familiarity with the current Makefile, so I was looking to do the partial replace of kit by moving foreground tasks into Make. #12583 is one effort toward that -- I still need to fix some race conditions with that, but the other potential con is that there's no fancy log handling for parallel tasks in Make.

I think reducing the scope of kit to background processes would be good, but the question of what tool to move to is still up in the air. Does anyone have thoughts on using Make or replacing it?

Should we close this?

Otherwise I think we can close this since task is not a full kit replacement and goreman is not desired to replace the other portion of kit. Especially if we don't want to replace Make with task, then I think there's no reason to keep this open, other than some discussion of the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants