Add findInPath method to locate all matching executables in the system path by smorimoto · Pull Request #609 · actions/toolkit

smorimoto · 2020-10-19T05:25:14Z

David is the original author. This PR is mainly for #610.

smorimoto · 2020-10-19T05:44:28Z

Could you check this? @ericsciple @bryanmacfarlane

smorimoto · 2020-10-20T10:50:15Z

CC: @konradpabjan @damccorm

smorimoto · 2020-10-22T04:06:43Z

We won't be able to work on fixing a lot of cache-related issues until we've done this.

smorimoto · 2020-10-29T07:48:04Z

@joshmgross Hi Josh. Sorry to keep pinging you all, but this is one of the steps needed to speed up actions/cache and avoid some bugs. Could you check it?

smorimoto · 2021-01-27T06:52:45Z

ping

smorimoto · 2021-01-28T05:11:46Z

@dhadka as well as #698

Be-ing · 2021-02-07T22:11:23Z

ping

smorimoto · 2021-02-14T04:01:02Z

@joshmgross @dhadka When using cache action on Windows with the Cygwin environment, we have to merge the PR that requires this change, and the delay in reviewing this is literally blocking all those tasks.

Be-ing · 2021-02-25T17:35:08Z

ping! Please prioritize whatever it takes to use zstd reliably on Windows. Decompressing a 1 GB cache on Windows takes 5 minutes per job!

smorimoto · 2021-03-05T20:36:29Z

@dhadka Can you assign a person about this? Other team members seem to have no interest in working on this topic.

dhadka

This looks fine to me.

dhadka · 2021-03-09T16:23:24Z

@actions/actions-runtime Can we please get a review on this PR?

konradpabjan

Changes look good.

If you could add some tests though that would be great! Good to have for future maintenance. We have some existing ones here: https://github.com/actions/toolkit/blob/main/packages/io/__tests__/io.test.ts

smorimoto · 2021-03-10T16:02:54Z

@konradpabjan I just added the simplest test.

smorimoto · 2021-03-10T16:40:28Z

There are a lot of irrelevant changes after I changed what I was concerned, so I will do them in another PR. Anyway, this PR is ready to merge.

dhadka · 2021-03-10T19:56:41Z

Thanks! @konradpabjan can you please give this another look and a ✔️ for actions-service if OK? findInPath is used by io.which so it should have good test coverage in addition to the test @smorimoto added.

smorimoto · 2021-03-10T20:37:22Z

Actually, the coverage isn't as bad as we thought.

------------|---------|----------|---------|---------|----------------------------------------------------------------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------|---------|----------|---------|---------|----------------------------------------------------------------------------------
All files | 74.02 | 65.96 | 94.12 | 75.63 |
io-util.ts | 64.2 | 53.85 | 87.5 | 64.1 | 29,50,54,125,133-135,147,149-151,153,155,161,162,164-170,175,180,182,183,196,199
io.ts | 80.49 | 73.03 | 100 | 83.19 | 54,69,98,117-119,121,126,130,131,135,182,204,215,221-223,323,327,328
------------|---------|----------|---------|---------|----------------------------------------------------------------------------------

smorimoto · 2021-03-10T20:39:26Z

@dhadka As for actions-service, the review request is made because of the unrelated commit (I already excluded them), so we can ignore it.

konradpabjan

Looks good to me! Thanks for adding some tests. Going to ask one more person for a quick 👀 (runtime team) though before merging this in and publishing

smorimoto · 2021-03-11T14:18:11Z

Thanks again 🙂

thboop

Left some minor comments but lets get a new version of this out once they have been addressed!

…m path Signed-off-by: Sora Morimoto <sora@morimoto.io>

smorimoto · 2021-03-15T18:24:01Z

@thboop now all points have been fixed!

smorimoto · 2021-03-16T19:38:25Z

Please let me know if there is anything else I can do to finish.

smorimoto · 2021-03-18T01:25:27Z

ping @dhadka @thboop

dhadka · 2021-03-20T00:23:49Z

Changes LGTM, ready to 🚀 once @thboop gives the 👍

smorimoto · 2021-03-23T04:26:44Z

ping @thboop

smorimoto · 2021-03-24T16:29:52Z

Hi? @thboop

thboop · 2021-03-24T17:04:20Z

Hey @smorimoto ,

I'll take a look at this next week and see if we can get a new release out. We typically have longer turnaround times for new features in open source repositories, as any feature requires further maintenance and support in the long term by our team. I'll be updating the repo with some templates to set better guidance for contributors in the future, I'm sorry for the churn this has caused you.

smorimoto · 2021-03-25T10:42:24Z

Thanks. I can understand the reason itself, but to be honest I'm quite skeptical about the fact that it would be a reasonable reason for hardly do a review for 6 months. And that seems to conflict with the reason why we do versioning in software in general. In any case, I hope we can make a "fix" (not an improvement!) for actions/cache as soon as possible.

Be-ing · 2021-03-25T18:57:33Z

I also think you should reconsider your processes if it takes 6 months to review a rather small pull request.

smorimoto · 2021-03-29T19:25:52Z

I'm glad I'm not the only one who feels that. In fact, the most PRs sent to the Actions repositories are well-known to require 100 pings to move the review process forward. This is a real loss for both GitHub and its users.

smorimoto · 2021-04-02T15:49:27Z

ping?

thboop

Changes Look great LGTM

dhadka · 2021-04-02T16:23:45Z

🚀

dhadka · 2021-04-02T16:24:10Z

@konradpabjan Can we please get a new version of this built?

thboop · 2021-04-02T16:24:19Z

Thanks for the contribution @smorimoto and being patient with the delay, working on a new npm release now 🚀 🚀

smorimoto · 2021-04-02T16:33:11Z

That was quick.

thboop · 2021-04-02T17:03:51Z

@actions/io v1.1.0 has been published 🚀 🚀 🚀

smorimoto mentioned this pull request Oct 19, 2020
Prefer using GNU tar #610
Closed

smorimoto force-pushed the io-findinpath branch from 4a9d190 to f164a52Compare October 19, 2020 05:43

joshmgross requested review from ericsciple and thboop October 29, 2020 15:53

smorimoto force-pushed the io-findinpath branch from f164a52 to e8f0755Compare January 27, 2021 07:05

smorimoto requested a review from a team as a code owner January 27, 2021 07:05

smorimoto force-pushed the io-findinpath branch 2 times, most recently from 9468eb1 to 9db7742Compare January 27, 2021 07:06

dhadka approved these changes Mar 9, 2021
View reviewed changes

dhadka requested a review from konradpabjan March 9, 2021 16:21

konradpabjan reviewed Mar 9, 2021
View reviewed changes

smorimoto force-pushed the io-findinpath branch 2 times, most recently from 0364793 to aa6bdefCompare March 10, 2021 16:01

smorimoto requested a review from a teamMarch 10, 2021 16:16

smorimoto force-pushed the io-findinpath branch 2 times, most recently from e1b358a to aa6bdefCompare March 10, 2021 16:39

smorimoto force-pushed the io-findinpath branch from aa6bdef to 3d1f533Compare March 10, 2021 16:49

konradpabjan approved these changes Mar 10, 2021
View reviewed changes

thboop reviewed Mar 11, 2021
View reviewed changes

Comment threadpackages/io/src/io.ts
Comment threadpackages/io/src/io.ts Outdated
Comment threadpackages/io/src/io.ts

Add findInPath method to locate all matching executables in the syste…
4db2f60
…m path Signed-off-by: Sora Morimoto <sora@morimoto.io>

smorimoto force-pushed the io-findinpath branch from 67cc5b1 to 4db2f60Compare March 12, 2021 04:57

smorimoto requested a review from thboop March 12, 2021 05:01

smorimoto mentioned this pull request Mar 28, 2021
setup-ocaml v2 ocaml/setup-ocaml#66
Merged
4 tasks

thboop approved these changes Apr 2, 2021
View reviewed changes

thboop merged commit bd9017e into actions:mainApr 2, 2021

smorimoto deleted the io-findinpath branch April 2, 2021 16:33

Uh oh!

Conversation

smorimoto commented Oct 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smorimoto commented Oct 19, 2020

Uh oh!

smorimoto commented Oct 20, 2020

Uh oh!

smorimoto commented Oct 22, 2020

Uh oh!

smorimoto commented Oct 29, 2020

Uh oh!

smorimoto commented Jan 27, 2021

Uh oh!

smorimoto commented Jan 28, 2021

Uh oh!

Be-ing commented Feb 7, 2021

Uh oh!

smorimoto commented Feb 14, 2021

Uh oh!

Be-ing commented Feb 25, 2021

Uh oh!

smorimoto commented Mar 5, 2021

Uh oh!

dhadka left a comment

Choose a reason for hiding this comment

Uh oh!

dhadka commented Mar 9, 2021

Uh oh!

konradpabjan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smorimoto commented Mar 10, 2021

Uh oh!

smorimoto commented Mar 10, 2021

Uh oh!

dhadka commented Mar 10, 2021

Uh oh!

smorimoto commented Mar 10, 2021

Uh oh!

smorimoto commented Mar 10, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

konradpabjan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smorimoto commented Mar 11, 2021

Uh oh!

thboop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smorimoto commented Mar 15, 2021

Uh oh!

smorimoto commented Mar 16, 2021

Uh oh!

smorimoto commented Mar 18, 2021

Uh oh!

dhadka commented Mar 20, 2021

Uh oh!

smorimoto commented Mar 23, 2021

Uh oh!

smorimoto commented Mar 24, 2021

Uh oh!

thboop commented Mar 24, 2021

Uh oh!

smorimoto commented Mar 25, 2021

Uh oh!

Be-ing commented Mar 25, 2021

Uh oh!

smorimoto commented Mar 29, 2021

Uh oh!

smorimoto commented Apr 2, 2021

Uh oh!

thboop left a comment

smorimoto commented Oct 19, 2020•
edited
Loading

konradpabjan left a comment •
edited
Loading

smorimoto commented Mar 10, 2021•
edited
Loading

konradpabjan left a comment •
edited
Loading