Skip to content

Conversation

@shalvah
Copy link
Contributor

@shalvah shalvah commented Feb 13, 2022

I noticed that whenever I run sls invoke or sls offline, the serverless_rack and rack_adapter files are deleted. This is extremely frustrating, as it means I have to run rack install every time I wish to test my function, which is a pretty expensive operation—I'm not using Docker, and it takes up to two minutes at times, slowing down my entire workflow.

There isn't really a need for this, IMO. I think cleanup should be left to the end user.

@logandk
Copy link
Owner

logandk commented Apr 4, 2022

Thanks for the PR. Originally these files (serverless_rack.rb and rack_adapter.rb) only made sense in the context of deployment, which is why they were being cleaned up. With sls invoke and sls offline, it obviously makes sense to keep them, as you've suggested.

I'd be hesitant to just change the default behavior like this, and at the very least, it'd require a major version bump. Instead, for now, I think it should be an opt-in configuration option. Would that work for you?

@shalvah
Copy link
Contributor Author

shalvah commented Apr 5, 2022

Hm, I'm kinda reluctant to add config, because it doesn't seem a significant enough change for that.

IMO, this is a default that is worth changing. Deleting the files in the background is an anti-pattern, when you think about it. We generated these files explicitly by running rack install, and then all of a sudden, _I can't find them on my computer. Wut?

Also, I don't think "cleaning up" provides much value. The files are regenerated on each deploy, so there's nothing to gain from deleting them afterwards. And they should definitely not be magically deleted after invoke:local is run.

Sure, we can fo a new major. I recommend adding a rack:cleanup command for those who actually wish to "clean up". I can work on that, adding support for Serverless v3 or any minor issues if we need a justification for a new major.

@logandk
Copy link
Owner

logandk commented Apr 5, 2022

Thanks, sounds good.

Regarding the change in default, my opinion is that users should be made aware that they'll need to add the generated files to their .gitignore. Preferably both a note in the README and a message at install-time, if possible through the install hook.

@shalvah
Copy link
Contributor Author

shalvah commented Apr 5, 2022

We can add a note that they can do so, but they don't need to. Whatever they were doing in the past will still work, since the deployment hook exists. This doesn't change anything.

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