Remove unnecessary env variables passing #1560
Conversation
docker-compose.yml
Outdated
There was a problem hiding this comment.
This is not related to this PR and we're not still sure if there is a problem at all, as no one else has reported this issue until now. Let's keep the discussion in #1556.
There was a problem hiding this comment.
My mistake, yep. Removed this commit from PR.
install/build-docker-images.sh
Outdated
There was a problem hiding this comment.
Doesn't this break current users' workflow using http_proxy env variable?
There was a problem hiding this comment.
Yes you are right. But I think it's very specific scenario: such users can access the Dockerhub without any proxy, but need http proxy for some resources during Sentry installation, like https://github.com/getsentry/wal2json.
For users who access any Internet resources (including Dockerhub) during installation it's required to set HTTP Proxy several times:
- Docker systemd file for
docker pull ~/.docker/config.jsonfor any commands inside docker containers/etc/environmentfor settings shell proxy variables
I think we can set only do 1. and 2. But in this case docker build will fail because in the commands above we explicitly overwrite proxy variables with empty values, that is why I suggest to modify these lines.
Actually we can keep these lines unchanged but from our discussion getsentry/develop#622 (comment) I'd found that this behavior is a bit unclear, that is why I'd created this PR with such changes.
There was a problem hiding this comment.
Let me check it one more time. I'd tried to install Sentry with my changes again and there are some issues.
There was a problem hiding this comment.
I'd rechecked this one more time. Proxies from ~/.docker/config.json work only for docker run command and do not work for docker build. It is written in the docker docs:
When you create or start new containers, the environment variables are set automatically within the container.
So we have to pass proxy env variables explicitly for the build command, that is why this PR shouldn't be merged.
install/install-wal2json.sh
Outdated
|
@goganchic Thank you very much for your effort and getting to the bottom of this! |
2 participants
PR into docs: getsentry/develop#628
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.