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

Add support for ignorefile when copying project #255

Closed
wants to merge 5 commits into from

Conversation

pelmered
Copy link

@pelmered pelmered commented Jun 11, 2024

This PR adds support for a .vaporignore file that lets you control what files are copied over to the .vapor build folder.

Background

I have a project with a DDD-structure as is pretty common with Laravel nowdays. We recently started to put test files inside each domain and that causes problems with the domian autoloader we have. It scans though the domain code and when i loads the test files we get an error that the TestCase file can't be found becuase the whole /test directory is not copied to the .vapor build folder.

While this is a very specific problem that can be solved in other ways, I got the idea to make vapor support an ignore file that lets you control what files are copied over to the .vapor build folder.

Why

  • This lets you control what files are copied and what files will be bundled in the artifact that is uploaded to AWS. It is important to try to keep the artifact small to save money and you might also not need to build a docker container if you can ignore more files.
  • This significantly speeds up the deploys. The copying step in our application when down from about 10-12 seconds to 1 second or less (we have some SQL dumps that I think slows things down a bit). The later steps for uploading is obviously also faster but I haven't benchmarked that.
  • It saves some disk space.

Other considerations

Maybe the .idea and /tests excludes in ApplicationFiles should be removed as well to let the user control this, but that would probably be considered a breaking change. We could add this as a default if no .vaporignore file is found to keep the current behavior. Maybe the same could go for the other lines in there as well, such as frankenphp, rr(roadrunner)

TODO

Write documentation, and maybe provide an example file/stub that can be published. I can write this if this gets accepted.
I would also like to add some tests, but that would require orchestral/testbench to make a proper test setup with ignore file.


This is what the .vaporignore looks like in our project:

/tests/
/src/Domains/*/Tests/
/src/Domains/*/Database/Seeds/
/docs
/.run
/.github
/.phpunt.cache
/reports
/scripts
/stubs
/http

/database/dumps
/database/schema
/database/sql

@taylorotwell taylorotwell marked this pull request as draft June 11, 2024 19:11
@joedixon
Copy link
Contributor

@pelmered is there a reason you can't use the ignore key in the vapor.yml file?

id: 1
name: my-app-name
ignore:
  - my-folder

@pelmered
Copy link
Author

pelmered commented Jun 17, 2024

@pelmered is there a reason you can't use the ignore key in the vapor.yml file?

id: 1
name: my-app-name
ignore:
  - my-folder

Sorry, I missed this comment.
I did not know about that feature. It is not documented and I didn't find it when I followed the code since it runs so late in the deploy process.

That is the problem with that solution. It runs so late in the process. The files are first copied to the build directory and then they are deleted. This is very slow and inefficient. The files should not be copied in the first place. I also think what happens in ApplicationFiles should be more configurable, and not a bunch of hard coded values.

In my case I also need the files to be removed before the build step because the domain tests are autoloaded but broken when the /tests directory does not exists. That makes my build fail.

However, there are probably use cases where you want files deleted after the build step as well so that should probably still be possible in some way. You can of course do that as the last step of the build process if you need.

Maybe the .vaporignore file should be skipped and everything should be configured in vapor.yml. I think it makes sense to have it in a separate file, but I would be fine with that as well. One suggestion would be to have the ignore filter the files copied and then maybe a cleanup configuration that runs after the build step. That could be used to cleanup any unwanted artifacts from the build step for example. That would of course be a breaking change. Are there any plans for a version 2 at the moment?

@joedixon
Copy link
Contributor

Thanks for the additional context @pelmered.

I'm going to close the PR for now and add this to the backlog internally for the team to discuss further. We don't have plans for a v2 right now.

@joedixon joedixon closed this Jun 20, 2024
@pelmered
Copy link
Author

Thanks for the additional context @pelmered.

I'm going to close the PR for now and add this to the backlog internally for the team to discuss further. We don't have plans for a v2 right now.

Ok, too bad.

It would be great if you at least could hook into ApplicationFiles:get() in some way to add custom filters. That would be easy to implement and not break any backwards compatibility. Otherwise, I have have to maintain my own fork to make our project work with Vapor at all.

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