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

Copy node_modules folder when symlink fails #24

Closed
wants to merge 3 commits into from

Conversation

josenicomaia
Copy link

The "solution" is to copy the entire folder when symlink fails.

Solution for #23

The "solution" is to copy the entire folder when symlink fails.

Solution for serverless#23
@schickling
Copy link
Contributor

@pmuens would you mind taking a look? Is this a problem/pattern you saw in other plugins as well?

@pmuens
Copy link

pmuens commented Jun 28, 2017

@schickling interesting 🤔

Thanks for pointing me in that direction. I remember that we had some issues with symlinking in the core (in early v1 versions). But they were resolved since then.

Have not (specifically) heard about others plugins dealing with that kind of problem though.

@schickling
Copy link
Contributor

So what's your suggestion @pmuens? Does this need to be addressed in the Serverless core?

@pmuens
Copy link

pmuens commented Jun 29, 2017

So what's your suggestion @pmuens? Does this need to be addressed in the Serverless core?

Would be nice if we could write a test which reproduces this behavior in core to ensure that's it's a bug in that codebase. After validating that we can fix it over there.

@schickling
Copy link
Contributor

@josenicomaia would you mind taking a look at this? If it turns out to be a limitation of the typescript plugin, I'm more than happy to merge the PR. I just want to make sure we're fixing the actual problem here.

@josenicomaia
Copy link
Author

josenicomaia commented Jun 29, 2017

It's not a core bug. It's a windows limitation that does't have the permission to create symbolic links.
I thought that if we don't have the permission, it is better copy instead of breaking.

About the test, I can make it to cover this case, but it will not expose the problem as I will have to mock fs.symlinkSync() and make it throw an exception.

Is that what you are expecting?

@schickling
Copy link
Contributor

I see. Could you please add another comment documenting that this is a Windows limitation?

@josenicomaia
Copy link
Author

Sure I do.

There is more details on the related issue. I have updated it. #23

@schickling
Copy link
Contributor

Sorry if I wasn't clear enough. I meant adding a comment to the code itself 🙂

- Added an information comment about the windows issue
@josenicomaia
Copy link
Author

@schickling I partially refactored the deployment routine. What do you think?

import * as path from 'path'
import * as fs from 'fs-p'

export class DeployBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that you pulled out this complexity to a separate file but I don't think that the name is intuitive and it doesn't need to be a class.

I'd suggest you export a function called symlink instead of a class and use all of the functions outside of the scope of a class.

Does that make sense to you? :)

Copy link
Author

Choose a reason for hiding this comment

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

Sure. It does :)
The purpose of this class is to prepare the deployment. It just happens that I refactored a little piece of it. I intended to put all the deployment part on it.
What do you think? Should I complete the refactoring or keep it in the monolite way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's great that you're splitting it up into multiple functions, I just think they don't need to live inside a class.

@natesilva
Copy link

Will this be merged soon? My Windows developers are currently working around the problem by using an Administrator-level prompt.

@schickling
Copy link
Contributor

@pmuens can you provide some update regarding this issue? Is this something that should be handled by the core of the serverless framework?

@pmuens
Copy link

pmuens commented Sep 11, 2017

Thanks for pinging @schickling 👍

🤔 So since this is more like a feature rather than a bug I'd suggest to open up an issue at serverless/serverless so that we can discuss it over there.

Generally speaking I've not yet encountered such a feature proposal in the Serverless Framework core repository. Might be interesting to see how other Windows users think about this!

@remss
Copy link

remss commented Nov 6, 2017

The symlink on package.json complains too. It should be copied like the node_modules does if EPERM is raised.

@0zguner
Copy link

0zguner commented Jan 9, 2018

any update on this ?

@EugeneDraitsev
Copy link

Any updates about this issue?

@JackCuthbert
Copy link
Contributor

A fix for this has been released in version 1.1.7 🎉

Please re-open a new issue if you think this is still relevant 😄

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.

8 participants