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

MYST-413 Fix broken Alpine image #197

Merged
merged 10 commits into from Mar 12, 2018

Conversation

Projects
None yet
3 participants
@Waldz
Copy link
Member

commented Mar 12, 2018

No description provided.

@Waldz Waldz requested review from tadovas, donce and zolia Mar 12, 2018


# Install application
COPY bin/client_docker/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Member

From docker perspective it's not very efficient to put executable AND it's args into entry point, as it will be very hard to override params later. It should be like this:
ENTRYPOINT ["your exec and args which must be always present and never change"]
CMD["default additional args which can be customized when using docker run style"]
those could be tequila.port , address , discovery address etc.
Do we really even need docker-entrypoint.sh ? it hides important stuff, also uses env vars internally and it's hard to track what is going on

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Author Member

Good ideas, will depreciate Docker environments variables in upcoming PRs

cap_add:
- MKNOD
- NET_ADMIN
networks:

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 12, 2018

Member

You don't need to force use default network. Each docker compose creates and maintains it's own network automatically

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Author Member

Removed network injection

@@ -13,7 +13,7 @@
#> GOOS=windows GOARCH=amd64 bin/client_build
#
# Check if program has dynamic libraries:
#> readelf -d /build/client/mysterium_client
#> readelf -d build/client/mysterium_client

This comment has been minimized.

Copy link
@zolia

zolia Mar 12, 2018

Member

There are no readelf on osx by default. We should add help notice how to get it.

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 12, 2018

Author Member

Fixed

Waldz added some commits Mar 12, 2018

@tadovas
Copy link
Member

left a comment

Looks good so far

@Waldz Waldz merged commit 33eb477 into master Mar 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the feature/MYST-413-docker-alpine branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.