Adding .gitignore support in project-archiver #463
Conversation
|
@alexalikiotis I think this looks good so far. Looks like the parse-gitignore has no dependencies, so that is good. I would go for it for adding the tests |
lib/project-archiver.js
Outdated
| ]; | ||
| // Parse ignore files | ||
| const gitignoreRules = await helpers.parseIgnoreFile(`${config.projectLocation}/.gitignore`); | ||
| exclusionRules.push.apply(exclusionRules, gitignoreRules); |
There was a problem hiding this comment.
you can also merge arrays like this too: const merged = [...array1, ...array2];
I wouldn't change this now, since there are other places in this file that do the same thing you are doing. I think when i first created nodeshift, that syntax wasn't available yet. I can add another issue for use to look at using spread to merge things
There was a problem hiding this comment.
Yeah, I used this method so I'll be consistent with the rest of the file.
| const file = await readfile(path); | ||
| return parse(file); | ||
| } catch (e) { | ||
| return []; |
There was a problem hiding this comment.
Not following why this needs to return an empty array in case of error, other than that seems good.
There was a problem hiding this comment.
i don't think we can assume that a user is using git, so if they are not, we don't want to just fail, and the empty array plays into the "array merge" in the project-archiver
There was a problem hiding this comment.
thanks for the information 👍
|
@lholmquist I've done some small updates and wrote some tests. I think the PR it's good to go! |
There was a problem hiding this comment.
@alexalikiotis nice job.
The only thing is that the exclusion rules could have multiple of the same value, but i don't think that is really to much of a big deal.
lholmquist
dismissed
their stale review
Just noticed something that i think we need to address
There was a problem hiding this comment.
It looks like right now, it will always do the gitignore and dockerignore even if a users uses the files property in the package.json.
I think with that case(using the file props), then we should skip looking at the ignore files since they are explicitly saying they want only these files
|
@lholmquist are you sure ?? Cause the project-archiver checks (line 32) if the files property is specified. Only if it's not, continues to the default, gitignore, and dockerignore rules. |
I'll try to fix that before we merge it. |
nvm, i was looking at it in the github preview, so that part wasn't expanded 🤦 |
This PR adds .gitignore file support in nodeshift's project archiver. #460
Two new NPM packages have been used:
.dockerignore support should be 2-3 extra lines of code since the core implementation is done.
@lholmquist @helio-frota I'm just pinging you here cause I can't add any reviewers for this PR
If you guys think everything is OK I'll continue with the .dockerignore support and tests.