Skip to content

improvment: reduce dist size #189

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

Merged
merged 2 commits into from
Jun 19, 2019

Conversation

Jibbarth
Copy link
Collaborator

Q A
Bug fix? no
New feature? yes
Fixed tickets #...

Following this article ("Use gitattributes to keep your tests out of other people’s production") I added some missing files to .gitattributes export-ignore

On the same idea, I complete the dockerignore also.
Note: I didn't exclude stub folder for those who get phpinsights through packagist, but I excluded it from docker because there is no way for docker user to retrieve stub from the container.

@nunomaduro nunomaduro requested a review from olivernybroe June 18, 2019 19:50
Copy link
Collaborator

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

It looks really good. And makes sense to reduce the package.

You should be able to ssh into the docker or use the cp command to extract files. (not docker expert so I might be wrong)
However the docker was created for running insight, not getting the stubs, so I think
that removing them makes sense. If a user really needs them, he can always fetch
them from github.

@nunomaduro nunomaduro merged commit 0490646 into nunomaduro:master Jun 19, 2019
@Jibbarth
Copy link
Collaborator Author

Thank you @olivernybroe for the review.

To be honest, I was unable to launch a custom command on the container 😅
And as far I know, you cannot ssh into a container, until an ssh server is installed and launched inside.

 $ docker run -it -v $(pwd):/app nunomaduro/phpinsights /bin/sh 
                                     
  Command "/bin/sh" is not defined.                                    

 $ docker run -it -v $(pwd):/app nunomaduro/phpinsights /bin/ls
                                     
  Command "/bin/ls" is not defined.                                    

 $ docker run -it -v $(pwd):/app nunomaduro/phpinsights ls     
                                
  Command "ls" is not defined.             

But maybe you have some tricks ?

@Jibbarth Jibbarth deleted the feature/reduce-dist-size branch June 19, 2019 19:08
@olivernybroe
Copy link
Collaborator

olivernybroe commented Jun 19, 2019

@Jibbarth was the same I was thinking on doing. I am on a phone right now so can't test myself, but as it is Alpine based, what if you try with ash instead of sh, and try sh directly instead of full path 🤔

@Jibbarth Jibbarth mentioned this pull request Jun 20, 2019
7 tasks
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.

3 participants