Page MenuHomePhabricator

[Services] Use GNU + BSD compatible find command
ClosedPublic

Authored by jon on Jan 6 2023, 3:26 PM.
Tags
None
Referenced Files
F3523341: D6192.diff
Mon, Dec 23, 9:00 AM
Unknown Object (File)
Sat, Nov 30, 4:04 PM
Unknown Object (File)
Sat, Nov 30, 10:11 AM
Unknown Object (File)
Thu, Nov 28, 4:06 AM
Unknown Object (File)
Thu, Nov 28, 4:06 AM
Unknown Object (File)
Sun, Nov 24, 2:43 PM
Unknown Object (File)
Nov 22 2024, 4:10 AM
Unknown Object (File)
Nov 22 2024, 4:10 AM
Subscribers

Details

Summary

Use -type d to make the command not error with GNU findutils.

Part of https://linear.app/comm/issue/ENG-2651

Test Plan
On a mac, which has BSD Tools
$ cd services
$ ./scripts/list_services.sh

$ nix develop
$ ./scripts/list_services.sh

# Should produce the same output

# Run calling script
yarn run-integration-tests blob

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek requested changes to this revision.Jan 6 2023, 10:59 PM

This works, but the output is different than original:

# original output
identity
tunnelbroker
backup
blob

# new output
./identity
./tunnelbroker
./backup
./blob

This impacts other commands using it: yarn run-integration-tests blob fails with

No such service: blob
Expected one of these:
./identity
./tunnelbroker
./backup
./blob
all

This isn't a big issue though, as yarn run-integration-tests ./blob works but this probably requires updating some docs and testing other commands. So stripping ./ would be a better option

This revision now requires changes to proceed.Jan 6 2023, 10:59 PM

Can you amend the test plan to include calling yarn run-integration-tests ./blob? Remember my advice about increasing the scope of your test plan... you don't just want to test the immediate change you're making, you want to test the things it affects

Also please git grep through the project to see where else this command is being used, and add all of the results to the Test Plan. I really think it's what you should aim for on every diff, and that it's not excessive

Update:
Noticed that adding ./ to service name when calling yarn run-integration-tests ./blob doesn't fix the issue as no tests are matched (the inner cargo test "./blob" command succeeds with 0 tests being run).

This works, but the output is different than original:

Oops, that was my bad

Also please git grep through the project to see where else this command is being used, and add all of the results to the Test Plan. I really think it's what you should aim for on every diff, and that it's not excessive

Only other services place is ran within docker image, which is already GNU find

$ rg 'find '
...
services/lib/docker/run_service.sh
7:EXE=$(find $EXE_PATH -mindepth 1 -maxdepth 1 -not -path '*/.*')

This is already used within the nix context

nix/start-powerline.sh
60:  find "$powerline_fonts" \

And this hook should already being ran (or maybe not, because clang support might not be 100%)

scripts/get_clang_paths_cli.js
9:    command += `find ${path} `;

Can you amend the test plan to include calling yarn run-integration-tests ./blob? Remember my advice about increasing the scope of your test plan... you don't just want to test the immediate change you're making, you want to test the things it affects

Amended, not sure why I didn't run the integration tests.

This one works well, thanks!

This revision is now accepted and ready to land.Jan 9 2023, 7:49 AM