search

cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
73 stars 20 forks source link

.cfignore file causing problems with the npm package tmp #45

Closed pmuellr closed 4 years ago

pmuellr commented 4 years ago

I just got a report that the .cfignore file in this repo is causing a problem when trying to use the npm package tmp.

https://github.com/cloudfoundry-community/node-cfenv/blob/4103a3e5185a53587444aafca9ee45029ebe50e0/.cfignore#L1-L3

I suppose that's kinda expected, and unfortunate, but I wonder how a .cfignore file in one package could impact another, unless that package is somehow installed underneath this one - it might be, from one of the other pre-reqs of this package. :shrug:

Anyhoo, it's probably a bit overkill to have this .cfignore file in here anyway, as it's used when you push this repo up as a CF app itself, to test it. It can live with some extra stuff ...

pmuellr commented 4 years ago

I believe @JenGoldstrich is going to fix this ...

JenGoldstrich commented 4 years ago

Hey @pmuellr

Yes! Will submit a commit adding an .npmignore shortly to minimize disruption, it'll just prevent the .cfignore from getting included in the node package.

For some context this is definitely undesirable on the cf-cli side but it is going to take us some time to think about how we want to fix how we scan for ignore files.

Thank you! Jenna

pmuellr commented 4 years ago

I'm actually not a fan of .npmignore files, as you then have to duplicate all the things they do by default.

It seems like deleting the .cfignore files will be easier, and the downside is very low (the image used for testing the package by pushing this repo will be a little bit bigger).

JenGoldstrich commented 4 years ago

Thats definitely a valid point, will delete the .cfignore!

JenGoldstrich commented 4 years ago

I have pushed v1.2.3 with this fix and published it to npm, closing this issue, thank you so so much again @pmuellr !

Jenna