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

improvment: reduce dist size #189

Merged
merged 2 commits into from Jun 19, 2019

Conversation

@Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Jun 18, 2019

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 Jun 18, 2019
Copy link
Collaborator

@olivernybroe olivernybroe left a comment

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.

Loading

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

@Jibbarth Jibbarth commented Jun 19, 2019

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 ?

Loading

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

@olivernybroe 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 🤔

Loading

@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants