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

Passing undefined as cwd to constructor causes strange behavior #45

Closed
JoshMcCullough opened this issue May 14, 2023 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@JoshMcCullough
Copy link
Contributor

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
Copy link
Collaborator

I suspect it might be related to this:

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

@shubhbapna shubhbapna added the bug Something isn't working label May 15, 2023
@JoshMcCullough
Copy link
Contributor Author

JoshMcCullough commented May 15, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants