kiegroup / act-js

A node.js wrapper for nektos/act to programmatically run your github actions locally
Apache License 2.0
53 stars 8 forks source link

Passing `undefined` as `cwd` to constructor causes strange behavior #45

Closed JoshMcCullough closed 1 year ago

JoshMcCullough commented 1 year ago

Describe the bug new Act(gh.repo.getPath("some invalid repo")) ends up passing undefined to the Act constructor. This, in my case, caused an error where (somehow) a .travis.yml file was being parsed by Act, with the following output:

Error: workflow is not valid. .travis.yml: yaml: unmarshal errors: line 28: cannot unmarshal !!seq into map[string]string

In this case, the .travis.yml file was located in a Node module at path: /path/to/my/project/node_modules/function-bind

To Reproduce Pass undefined to the constructore while you have an invalid *.yml file somewhere e.g. under node_modules.

Expected behavior The constructor probably should require a cwd, or throw if one is provided as null or undefined.

shubhbapna commented 1 year ago

I suspect it might be related to this:

I will try setting the --no-recurse flag and see if it solves this issue

JoshMcCullough commented 1 year ago

A flag could work, but it wouldn't really solve the issue here. The main question is this: is it ever valid to pass null or undefined as the path to the Act constructor? I can see it being valid if the intent is to use the current working directory as the path. If that is true, then perhaps Act needs to specifically avoid certain filenames such as ^\.travis\.ya?ml$.

JoshMcCullough commented 1 year ago

And if the answer is "yes, it is valid to provide a null/undefinfed path in the constructor", maybe that rules should be changed to require a path even if it's just ./.

shubhbapna commented 1 year ago

And if the answer is "yes, it is valid to provide a null/undefinfed path in the constructor", maybe that rules should be changed to require a path even if it's just ./.

So when we provide null/undefined path in the constructor then it implicitly means the current working directory - https://github.com/kiegroup/act-js/blob/main/src/act/act.ts#L29

However it seems that when you do it from the root of the directory which contains a node_module directory, act will go through and try to recursively detect workflow files.

JoshMcCullough commented 1 year ago

Yes, exactly. Not sure what the proper solution is here w/out forcing a non-null / non-undefined first arg. Probably would be annoying for others. I'm okay with chalking this up to "human error", you can close it if you'd like.

shubhbapna commented 1 year ago

Yes lets stick with convention that if no path is passed in the constructor then the current working directory will be used. I will update the readme.

Although it is annoying that act tries to recurse through node_modules. I will see if --no-recurse flag has any side effects.

JoshMcCullough commented 1 year ago

Sounds good.