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

Fix: Do not check in vendor directory #396

Closed
wants to merge 1 commit into from

Conversation

localheinz
Copy link
Contributor

This PR

  • adds vendor to .gitignore and removes checked in dependencies

πŸ’β€β™‚οΈ Not sure, is there a reason for them to be checked in? That's what we use composer for, right?

@joindin-jenkins
Copy link

Can one of the admins verify this patch?

@mrailton
Copy link

ok to test

@akrabat
Copy link
Contributor

akrabat commented Feb 17, 2017

vendor is checked in because the server does not have composer installed on it and the deployment scripts do not handle it.

When we set up to use composer, it did not do package signing or checksumming and we didn't trust that we couldn't guarantee that the code deployed via a script would be exactly what we tested against. I have no idea if this has changed.

@mrailton
Copy link

@localheinz thank you for the contribution. Accepting this will require some changes on the build test server as well as the test/production servers. This will need to be discussed with @liam-wiltshire and others before proceeding.

@akrabat
Copy link
Contributor

akrabat commented Feb 17, 2017

FWiW, composer/composer#1074 was only fixed last year and that was certainly one of the concerns when we first started using composer with joind.in.

@liam-wiltshire
Copy link
Member

For me, simply the fact that we might deploy to test but then not deploy to production for a little while justifies having it checked in. As much as revision changes shouldn't break things, I've still known them to!

This comes up every now and again, and the concensus is to stick with the status quo generally. Happy to discuss it further in IRC if anyone strongly objects, but as it stands I'd personally leave it as it is for now.

@akrabat akrabat closed this Feb 17, 2017
@localheinz
Copy link
Contributor Author

localheinz commented Feb 17, 2017

Ha, interesting - how about using composer then as part of the deploy pipeline?

@localheinz localheinz deleted the fix/vendor branch February 17, 2017 10:58
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.

5 participants