Skip to content

Remove unnecessary env variables passing #1560

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

Closed
wants to merge 1 commit into from
Closed

Remove unnecessary env variables passing #1560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions install/build-docker-images.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ echo "${_group}Building and tagging Docker images ..."
echo ""
# Build any service that provides the image sentry-self-hosted-local first,
# as it is used as the base image for sentry-cleanup-self-hosted-local.
$dc build --build-arg "http_proxy=${http_proxy:-}" --build-arg "https_proxy=${https_proxy:-}" --build-arg "no_proxy=${no_proxy:-}" --force-rm web
$dc build --build-arg "http_proxy=${http_proxy:-}" --build-arg "https_proxy=${https_proxy:-}" --build-arg "no_proxy=${no_proxy:-}" --force-rm
Copy link
Collaborator

@aminvakil aminvakil Jul 4, 2022

Choose a reason for hiding this comment

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

Doesn't this break current users' workflow using http_proxy env variable?

Copy link
Author

@goganchic goganchic Jul 4, 2022

Choose a reason for hiding this comment

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

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:

  1. Docker systemd file for docker pull
  2. ~/.docker/config.json for any commands inside docker containers
  3. /etc/environment for 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.

Copy link
Author

@goganchic goganchic Jul 4, 2022

Choose a reason for hiding this comment

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

Let me check it one more time. I'd tried to install Sentry with my changes again and there are some issues.

Copy link
Author

@goganchic goganchic Jul 4, 2022

Choose a reason for hiding this comment

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

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.

$dc build --force-rm web
$dc build --force-rm
echo ""
echo "Docker images built."

Expand Down
4 changes: 1 addition & 3 deletions install/install-wal2json.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ ARCH=$(uname -m)
FILE_NAME="wal2json-Linux-$ARCH-glibc.so"

docker_curl() {
# The environment variables can be specified in lower case or upper case.
# The lower case version has precedence. http_proxy is an exception as it is only available in lower case.
docker run --rm -e http_proxy -e https_proxy -e HTTPS_PROXY -e no_proxy -e NO_PROXY curlimages/curl:7.77.0 "$@"
docker run --rm curlimages/curl:7.77.0 "$@"
Copy link
Collaborator

@aminvakil aminvakil Jul 4, 2022

Choose a reason for hiding this comment

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

Also here.

}

if [[ $WAL2JSON_VERSION == "latest" ]]; then
Expand Down