8000 Symlink native composer home by Slamdunk · Pull Request #32 · chialab/docker-php · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Symlink native composer home #32

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 28, 2018
Merged

Symlink native composer home #32

merged 3 commits into from
Feb 28, 2018

Conversation

Slamdunk
Copy link
Contributor

Reference: #31 (comment)

@Nemo64
Copy link
Contributor
Nemo64 commented Feb 23, 2018

looks good.

A few notes on the PATH variable:

  • I think the PATH must be$(composer config --global home)/vendor/bin:$PATH. In the current implementation it would break if someone would mount the correct composer home since /root/composer would than be empty.
  • A bit of a site note but shouldn't the path be appended to the path variable? If it is prepended, some rouge package could install binaries with the name su or ssh and silently catch your login information.

@Slamdunk
Copy link
Contributor Author

I think the PATH must be $(composer config --global home)/vendor/bin:$PATH. In the current implementation it would break if someone would mount the correct composer home since /root/composer would than be empty.

$(composer config --global home) notation doesn't work in ENV docker statement, see #31 (comment)

A bit of a site note but shouldn't the path be appended to the path variable? If it is prepended, some rouge package could install binaries with the name su or ssh and silently catch your login information.

I agree. I just changed according to previous implementation, let's listen what @fquffio thinks.

@Slamdunk
Copy link
Contributor Author

To be honest I don't know if $(...) notation works in RUN statement either, I don't have the environment to test it at the moment.

@fquffio
Copy link
Collaborator
fquffio commented Feb 26, 2018

I didn't forget about this PR, but I hadn't time to take a look into it yet. I promise I'll do as soon as I can.

@Slamdunk $(cmd) does work in RUN instructions.

@Slamdunk
Copy link
Contributor Author
Slamdunk commented Feb 26, 2018 •
edited
Loading

I didn't forget about this PR, but I hadn't time to take a look into it yet. I promise I'll do as soon as I can.

Free and open-source software maintainers aren't requested to respect any schedule.

Take all the time you need 😉

@fquffio fquffio self-assigned this Feb 26, 2018
fquffio
fquffio approved these changes Feb 26, 2018
View reviewed changes
Copy link
Collaborator
@fquffio fquffio left a comment

Choose a reason for hiding this comment

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

Seems ok. Just two minor concerns… if you don't have time to take care of those no problem.

5.4/Dockerfile Outdated
RUN curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer
ENV PATH $COMPOSER_HOME/vendor/bin:$PATH
RUN ln -s $(composer config --global home) /root/composer
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd try to chain this command to the previous RUN:

RUN curl ... && ln -s ...

5.4/Dockerfile Outdated
RUN curl -sS https://getcomposer.org/installer | php -- --install-dir=/usr/local/bin --filename=composer
ENV PATH $COMPOSER_HOME/vendor/bin:$PATH
RUN ln -s $(composer config --global home) /root/composer
ENV PATH /root/composer/vendor/bin:$PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @Nemo64 that it would be better to have $PATH:/root/composer/vendor/bin, even if it used to be this way before.

@Slamdunk
Copy link
Contributor Author

Tomorrow morning I'll do the changes

@Slamdunk
Copy link
Contributor Author

Done

@fquffio fquffio merged commit fc79d49 into chialab:master Feb 28, 2018
@fquffio
Copy link
Collaborator
fquffio commented Feb 28, 2018

Thank you @Slamdunk ! Sorry if it took such a long time for me to get this merged.

@Slamdunk Slamdunk deleted the native_composer_home_symlink branch February 28, 2018 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fquffio fquffio fquffio approved these changes

Assignees

@fquffio fquffio

Labels
Non 4899 e yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Slamdunk @Nemo64 @fquffio
0