moonrepo / moon

A build system and monorepo management tool for the web ecosystem, written in Rust.
https://moonrepo.dev/moon
MIT License
2.88k stars 157 forks source link

[bug] fitler on change status does not work #414

Closed tomdavidson closed 1 year ago

tomdavidson commented 1 year ago

Describe the bug

Exploring moon's filtering to replace lint-staged githook and want to filter based on the status of the files changed. Unfortunately, the filtering on change status does not work.

Steps to reproduce

  1. Create a dummy root level task in //moon.yml

    ls:
    command: ls
    platform: system
  2. Make sure there are changed files in git with various status such as modified or staged.

  3. Run the task with the status filter as documented $ moon run root:ls --status staged

Expected behavior

The status filtering on staged should match the files that git shows are staged.

Environment

System: OS: Linux 5.11 Ubuntu 21.04 (Hirsute Hippo) CPU: (4) x64 Intel(R) Core(TM) i5-7300U CPU @ 2.60GHz Memory: 17.67 GB / 30.75 GB Container: Yes Shell: 5.1.4 - /bin/bash Binaries: Node: 18.11.0 - /usr/bin/node npm: 8.15.0 - ~/.local/bin/npm Managers: Apt: 2.2.4 - /usr/bin/apt Cargo: 1.64.0 - ~/.cargo/bin/cargo pip3: 20.3.4 - /usr/bin/pip3 Utilities: Make: 4.3 - /usr/bin/make GCC: 10.3.0 - /usr/bin/gcc Git: 2.30.2 - /usr/bin/git FFmpeg: 4.3.2 - /usr/bin/ffmpeg Virtualization: Docker: 20.10.14 - /usr/bin/docker VirtualBox: 6.1.26 - /usr/bin/vboxmanage IDEs: Nano: 5.4 - /usr/bin/nano VSCode: 1.72.2 - /usr/bin/code Languages: Bash: 5.1.4 - /usr/bin/bash Go: 1.16.6 - /usr/bin/go Perl: 5.32.1 - /usr/bin/perl Python3: 3.9.5 - /usr/bin/python3 Rust: 1.64.0 - /home/tom/.cargo/bin/rustc Browsers: Firefox: 96.0

Additional context

terminal output

```shell $ git status On branch ci/tune-up-eslint Your branch is up to date with 'origin/ci/tune-up-eslint'. Changes to be committed: (use "git restore --staged ..." to unstage) modified: configs/ts-node-config/README.md Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: .vscode/extensions.json modified: configs/eslint-config-skipa/package.json modified: configs/jest-config/package.json modified: configs/ts-config/package.json modified: eslint.config.js modified: package.json modified: pnpm-lock.yaml modified: towel.ts modified: tsconfig.json Untracked files: (use "git add ..." to include in what will be committed) .github/workflows/issues.yml .moon/ apps/docs/bob.ts apps/docs/eslint-print-config.json moon.yml renovate.json scratch.js tsconfig.options.json $ moon run :ls --status modified ▪▪▪▪ root:ls (cached) apps configs eslint.config.js moon.yml node_modules package.json packages pnpm-lock.yaml pnpm-workspace.yaml README.md renovate.json scratch.js towel.ts tsconfig.json tsconfig.options.json turbo.json Tasks: 1 completed (1 cached) Time: 16ms ❯❯❯❯ to the moon $ moon run :ls --status staged ▪▪▪▪ root:ls (cached) apps configs eslint.config.js moon.yml node_modules package.json packages pnpm-lock.yaml pnpm-workspace.yaml README.md renovate.json scratch.js towel.ts tsconfig.json tsconfig.options.json turbo.json Tasks: 1 completed (1 cached) Time: 12ms ❯❯❯❯ to the moon $ moon run :ls --status untracked ▪▪▪▪ root:ls (cached) apps configs eslint.config.js moon.yml node_modules package.json packages pnpm-lock.yaml pnpm-workspace.yaml README.md renovate.json scratch.js towel.ts tsconfig.json tsconfig.options.json turbo.json Tasks: 1 completed (1 cached) Time: 14ms ❯❯❯❯ to the moon ```
milesj commented 1 year ago

@tomdavidson All of the subsequent calls are cached, so its just hitting that. Are you expecting something else?

If you pass --log trace, what are the git commands that are running?

tomdavidson commented 1 year ago

@milesj OK sounds like I misunderstand the status filter. When I filter on status=changed, Im expecting to see the same files git status would show changed. When I filter on staged, I expect to see the same file git status shows are staged.

In this example, I filter on status=staged but it kinda looks like the output of git status --porcelain --untracked-files -z is not being filtered for stage and I am all the files from the git cmd.... and the list is the same regardless if I filter on status=modified, etc.

$ moon --log trace run root:ls --status staged 
...
[debug 10:42:35] moon:action:run-target Running target root:ls
[trace 10:42:35] moon:utils:process Running command git hash-object --stdin-paths - .moon/project.yml .moon/workspace.yml package.json (in /media/tom/toms-data/Projects/ramlaw/webapps)
[trace 10:42:35] moon:utils:process Running command git ls-tree HEAD -r . (in /media/tom/toms-data/Projects/ramlaw/webapps)
[trace 10:42:35] moon:utils:process Running command git status --porcelain --untracked-files -z (in /media/tom/toms-data/Projects/ramlaw/webapps)
[debug 10:42:35] moon:action:run-target Generated hash 356a55ff98e12240e1b41acc5e231da364804a17f136b2f9af9da7b8b0de7e3e for target root:ls
[debug 10:42:35] moon:action:run-target Cache hit for hash 356a55ff98e12240e1b41acc5e231da364804a17f136b2f9af9da7b8b0de7e3e, reusing previous build
▪▪▪▪ root:ls (cached)
apps
configs
eslint.config.js
moon.yml
node_modules
package.json
packages
pnpm-lock.yaml
pnpm-workspace.yaml
README.md
renovate.json
scratch.js
towel.ts
tsconfig.json
tsconfig.options.json
turbo.json
...
milesj commented 1 year ago

@tomdavidson Ok, so I completely missed this, but you also need to pass --affected. https://moonrepo.dev/docs/run-task#filtering-based-on-change-status

However, --status doesn't change the "hash inputs" (which you can see at .moon/cache/hashes), so subsequent runs will still hit the cache if the files in the working tree haven't changed status since the last run.

On a side note, I'll look into making those CLI args exclusive.

tomdavidson commented 1 year ago

@milesj interesting. I intentionally did not use --affected thinking it checks to see if files have changed and without it I could look at what --status does on its own. No that have used --affected --status staged no files are in the output at all and from the trace logging, git is not invoked. When I change a file and stage it, moon --log trace run root:ls --affected --status staged still shows no files. Also no change when I rm .moon/cache/hashes/*

milesj commented 1 year ago

@tomdavidson Is this available anywhere to test? If not, I can try and reproduce locally.

Also, you may want to try with cache off https://moonrepo.dev/docs/config/project#cache

tomdavidson commented 1 year ago

@milesj thanks for being willing to dig in on this - Im clearly not figuring it out. Here is a copy that can be public https://github.com/tomdavidson/moon-status-filter and I invited you as a collaborator.

I set the task to local and did not observe a difference.

milesj commented 1 year ago

@tomdavidson Awesome, will take a look.

While I'm digging into this, can you outline exactly what you are trying to achieve? And what the flow would look like.

tomdavidson commented 1 year ago

There is a handful of other filtering but the git based ones are:

precommit

Fast/light linting and formatting. Run prettier --write and eslint --fix task filter to only files that have status=staged (and git add)

prepush

Before pushing the changes, Run prettier --check and eslint as in precommit, but with env CI=true to enable the heavy eslint rules. Also run unit tests filtered to workspaces with changed files & their dependents.

CI

I would like to only run tasks in workspaces with files that have changed compared to main@HEAD (assuming a cloned working branch). I'm not sure if --affected --upstream does this because I have first been paying around with the githook workflows.

milesj commented 1 year ago

@tomdavidson I have noticed a few issues. I think it has to do with it being a root level project. I'll look into it as part of 0.19 work.

With that being said, there may be a misunderstanding in how this works. So the --affected/--status logic determines whether to run the task or not. It won't filter down the file list. So for example, if only 1 file was modified in a project, the eslint/prettier would still run on the entire project, not that 1 file. I have some ideas on how to solve this in the future using tokens (https://moonrepo.dev/docs/concepts/token) but it doesn't exist yet.

For your precommit/prepush scenarios, a potential solution could be using moon query touched-files (https://moonrepo.dev/docs/commands/query/touched-files). This returns a list of files that can then be passed to prettier and eslint directly. This would by pass the moon task flow, but is a stopgap solution.

For CI, that's exactly what the moon ci command is for. https://moonrepo.dev/docs/guides/ci

tomdavidson commented 1 year ago

@milesj --affected (a cross all commands) is a diff with the 3 potential comparisons where as --status is filtering affected (hence one uses --affected with --status)?

The task filtering does not output list of changed files.... That is how I expected it to work, but I feel a bit dumb (and apologetic) here. My dummy task to check things out calls ls and I started expediting the list of files to be the filtered files or workspace dirs.

Im currently running prettier as a root level task (partly because prettier can not find .editorconfig otherwise ) but eslint as a workspace level task (as with unit tests because it can be much more involved than formatting. moon query is a perfect for the root level prettier-like tasks (and eslint if Iit has the same usage). moon query looks very nice and I had overlooked it. I think the token or token functions to provide query output as input for the task workflow would be awesome.

But I am hopping to replace some of my scripts used run eslint and unit tests in each workspace that has changed files and be able to include dependents so Ill look for to 0.19 and hope I can do something like moon run lint --affected --status staged in a precommit hook.

moon ci is just right, I love it.

milesj commented 1 year ago

Yeah, I'll look into adding the $affectedFiles token in the next month or two. This should pair nicely with moon run :lint --affected --status staged as the pre hook and get you exactly what you want.

milesj commented 1 year ago

Was able to figure out the problem, and it was with root only projects. https://github.com/moonrepo/moon/pull/423

tomdavidson commented 1 year ago

423 nice, thanks for finding a solution so quickly.

Yeah, I'll look into adding the $affectedFiles token in the next month or two. This should pair nicely with moon run :lint --affected --status staged as the pre hook and get you exactly what you want.

niiiiiicce! Seriously, even though Im only poking around with moon on side, I am really diggin it. Thanks @milesj

milesj commented 1 year ago

Awesome, thanks for trying it and reporting this issue.

I'll probably throw $affectedFiles into the next release, since it's rather light.

milesj commented 1 year ago

The token approach wouldn't work, but still got something working: https://github.com/moonrepo/moon/pull/424

tomdavidson commented 1 year ago

Ya defining the query/filtering in the task definition would be quick slick but I think #424 is pretty great and will be perfect for my case.

milesj commented 1 year ago

@tomdavidson This has landed in the 0.19 release.