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

Make the output directories if not exists #49

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

rajbos
Copy link
Contributor

@rajbos rajbos commented Mar 26, 2023

I've bumped into this now three times: when the output directory does not exists (which it will not when the user starts with this), the action just crashes. I think it is a nicer experience to just create the directory for them: they indicate a path already where the files will be written to, so why not just make the dir for them as well? This PR handles that.

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea, @rajbos!

I will suggest to move this logic to an utility as it is used at least in two places in src/action.js. Also it will be easier to add unit tests later on (I can help you to mock fs methods if you need as Jest can be quite tricky)

Also it will be nice to handle recursive folders by using fs methods like mkdirSync('<the path>', {recursive: true}) and fs.existsSync(<PATH>). More context: https://joshtronic.com/2021/01/17/recursively-create-directories-with-nodejs/

Also you can use Sync methods (fs.exists vs fs.existsSync) in this case as we can block main thread as this is github action is more like an script than a I/O Service 👍

@UlisesGascon UlisesGascon added this to the v2.0.0-beta milestone Mar 28, 2023
@UlisesGascon
Copy link
Member

Solved conflicts in e1fec72

@rajbos
Copy link
Contributor Author

rajbos commented Apr 10, 2023

Moved the mkdir code into the utils class. Not sure about the next step: do you want to add unit tests now or later? When now, I do need some help with the setup of overwriting the fs class then.

I'd expect some tests for short/long file paths (multiple layers deep or not) as well as tests for when the folder already exists and when not?

@UlisesGascon
Copy link
Member

I can help you with the tests, I will add some commits to the PR :)

@UlisesGascon UlisesGascon changed the base branch from main to feat/create-directories April 11, 2023 09:27
@UlisesGascon
Copy link
Member

UlisesGascon commented Apr 11, 2023

@rajbos I will merge this PR to the branch feat/create-directories and from that branch I will add the tests and create a new PR against main.

See #53

@UlisesGascon UlisesGascon merged commit 1b257db into ossf:feat/create-directories Apr 11, 2023
@rajbos rajbos deleted the makedir branch April 11, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants