Repository: getsentry/sentry
Pull request: feat(direnv): direnv usability improvements
If the Python venv is not set up correctly we call `bootstrap-py3-venv` (we prompt the user first) even if it takes long. We call it once and we get into a good state.

Prompt when `.venv` is missing:
<img width="741" alt="image" src="https://user-images.githubusercontent.com/44410/116594050-cd4da500-a8ef-11eb-9266-6c0420d27410.png">

Something not being perfectly configured will still complete direnv's execution in order to have the virtual env activated. Instead, we will warn about what extra steps to take to bring everything up-to-date (See some warnings in yellow and commands to type in red):
<img width="727" alt="image" src="https://user-images.githubusercontent.com/44410/116593920-a0998d80-a8ef-11eb-92ad-fbcefe61ca64.png">

Other changes:
* Do not include `direnv: ` at the beginning of every line
* Do not bold every message
* Print debug messages if `SENTRY_DIRENV_DEBUG=1` is defined
* Add `make direnv-help` command to list documentation
<img width="663" alt="image" src="https://user-images.githubusercontent.com/44410/116593781-78119380-a8ef-11eb-9e90-6b08f1443fa3.png">
<img width="275" alt="image" src="https://user-images.githubusercontent.com/44410/116593748-6d56fe80-a8ef-11eb-9a67-885495a0ebf3.png">

Fixes #25578

EDIT:
* Updated screenshots
* Adjust some comments to reflect current state of PR

Problem statement:
direnv usability improvements
Suggested fixes:

- If there are steps that take short time simply take them.
- Give a list of commands in one shot (instead of requiring multiple direnv invocations) to get into a good state.
- Share code paths between `direnv` and `lib.sh`
- Take advantage of `bootstrap-py3-venv`

Hints from issue discussion:
for posterity: <img width="586" alt="image" src="https://user-images.githubusercontent.com/14209781/115909462-19659900-a45b-11eb-8666-c4129dc0895e.png">

Commit message: Comment adjustment

Patch to review:
diff --git a/.envrc b/.envrc
index ad98f8774cae27..b87a9c84062d99 100644
--- a/.envrc
+++ b/.envrc
@@ -19,6 +19,7 @@ trap notice ERR
 # This is used to group issues on Sentry.io.
 # If an issue does not call info() or die() it will be grouped under this
 error_message="Unknown issue"
+# This has to be the same value as what sentry-cli accepts
 log_level="info"
 
 notice() {
@@ -47,19 +48,23 @@ require() {
     command -v "$1" >/dev/null 2>&1
 }
 
-warn() {
+debug() {
+    if [ "${SENTRY_DIRENV_NOISY}" ]; then
+        echo "${@}"
+    fi
+}
+
+info() {
     cat <<EOF
-${yellow}${bold}direnv: ${@}${reset}
+${bold}${@}${reset}
 EOF
 }
 
-info() {
+warn() {
     cat <<EOF
-${bold}direnv: ${@}${reset}
+${yellow}${@}${reset}
 EOF
-    # info() calls happen multiple times in this script. The last info statement
-    # is the one that will be used to group Sentry issues
-    error_message=("${@}")
+    log_level="warning"
 }
 
 die() {
@@ -74,34 +79,10 @@ EOF
     return 1
 }
 
-advice_init_venv() {
-    venv_name="$1"
-
-    deactivate 2>/dev/null || true
-    info "To create a virtualenv, please type: python3.6 -m venv ${venv_name}"
-    require "python3.6" ||
-        die "You'll need to install python3, or make it available on your PATH.
-It's recommended to use pyenv - please refer to https://docs.sentry.io/development/contribute/environment"
-    return 1
-}
-
-advice_install_sentry() {
-    info "To install sentry, please type: make install-py-dev"
-    return 1
-}
-
-advice_install_pre_commit() {
-    info "To install pre-commit, please type: make setup-git"
-    return 1
-}
-
-advice_install_yarn_pkgs() {
-    info "To install yarn packages, please type: make install-js-dev"
-    return 1
-}
-
 ### Environment ###
 
+commands_to_run=()
+
 # don't write *.pyc files; using stale python code occasionally causes subtle problems
 export PYTHONDONTWRITEBYTECODE=1
 
@@ -124,20 +105,21 @@ export SENTRY_UI_HOT_RELOAD=1
 ### You can override the exported variables with a .env file
 # All exports should happen before here unless they're safeguarded (see devenv error reporting below)
 if [ -f '.env' ]; then
-    info ".env found. Reading it..."
+    debug "Loading variables from .env file..."
     dotenv .env
 fi
 
+# TODO: Ensure pre-commit requirements are up-to-date
 ## Notify of reporting to Sentry
 if [ -n "${SENTRY_DEVENV_NO_REPORT+x}" ]; then
-    info "No development environment errors will be reported (since you've defined SENTRY_DEVENV_NO_REPORT)."
+    debug "No development environment errors will be reported (since you've defined SENTRY_DEVENV_NO_REPORT)."
 else
     # Since direnv traps the EXIT signal we place the temp file under /tmp for the odd time
     # the script will use the EXIT path
     _SENTRY_LOG_FILE=$(mktemp /tmp/sentry.envrc.$$.out || mktemp /tmp/sentry.envrc.XXXXXXXX.out)
     exec > >(tee "$_SENTRY_LOG_FILE")
     exec 2>&1
-    info "Development errors will be reported to Sentry.io."$'\n'"        If you wish to opt-out, set SENTRY_DEVENV_NO_REPORT as an env variable."
+    debug "Development errors will be reported to Sentry.io. If you wish to opt-out, set SENTRY_DEVENV_NO_REPORT as an env variable."
     # This will allow `sentry devservices` errors to be reported
     export SENTRY_DEVSERVICES_DSN=https://23670f54c6254bfd9b7de106637808e9@o1.ingest.sentry.io/1492057
 fi
@@ -159,18 +141,19 @@ done
 
 ### Git ###
 
-info "Configuring git..."
-
 make setup-git-config
 
 ### Python ###
 
 venv_name=".venv"
 
-info "Activating virtualenv..."
+if [ ! -f "${venv_name}/bin/activate" ]; then
+    # This is time consuming but it has to be done
+    source ./scripts/bootstrap-py3-venv
+fi
 
 if [ ! -f "${venv_name}/bin/activate" ]; then
-    info "You don't seem to have a virtualenv."
+    echo "You don't seem to have a virtualenv."
     # If the version doesn't match the contents of .python-version it will fail
     [[ $(pyenv version) ]] ||
         die "pyenv does not have the right Python version installed. Please run:
@@ -182,34 +165,33 @@ fi
 # (in that it would be automatically deactivated) virtualenv active.
 deactivate 2>/dev/null || true
 
+# shellcheck disable=SC1091
 source "${venv_name}/bin/activate"
 
 # XXX: ideally, direnv is able to export PS1 as modified by sourcing venvs
 #      but we'd have to patch direnv, and ".venv" isn't descriptive anyways
 unset PS1
 
-info "Ensuring proper virtualenv..."
-${PWD}/scripts/ensure-venv.sh
+debug "Ensuring proper virtualenv..."
+"${PWD}/scripts/ensure-venv.sh"
 
 if ! require sentry; then
-    info "Your virtualenv is activated, but sentry doesn't seem to be installed."
-    # XXX: if direnv fails, the venv won't be activated outside of direnv execution...
-    # So, it is critical that make install-py-dev is guarded by scripts/ensure-venv.
-    advice_install_sentry
+    warn "Your virtualenv is activated, but sentry doesn't seem to be installed."
+    commands_to_run+=("make install-py-dev")
 fi
 
 ### pre-commit ###
 
-info "Checking pre-commit..."
+debug "Checking pre-commit..."
 
 if ! require pre-commit; then
-    info "Looks like you don't have pre-commit installed."
-    advice_install_pre_commit
+    warn "Looks like you don't have pre-commit installed."
+    commands_to_run+=("make setup-git")
 fi
 
 ### Node ###
 
-info "Checking node..."
+debug "Checking node..."
 
 if ! require node; then
     die "You don't seem to have node installed. Install volta (a node version manager): https://develop.sentry.dev/environment/#javascript"
@@ -218,21 +200,25 @@ fi
 make node-version-check
 
 if [ ! -x "node_modules/.bin/webpack" ]; then
-    info "You don't seem to have yarn packages installed."
-    advice_install_yarn_pkgs
+    warn "You don't seem to have yarn packages installed."
+    commands_to_run+=("make install-js-dev")
 fi
 
 PATH_add node_modules/.bin
 
-### Overrides ###
+# These are commands that can take a significant ammount of time
+if [ ${#commands_to_run[@]} -ne 0 ]; then
+    warn "Run the following commands to bring your environment up-to-date:"
+    for cmd in "${commands_to_run[@]}"; do
+        warn "    $cmd"
+    done
+fi
 
-cat <<EOF
-${green}${bold}direnv: SUCCESS!
-${reset}
-EOF
+### Overrides ###
 
-if [ -z "${SENTRY_SILENCE_DIRENV_NOTICE:-}" ]; then
+if [ -z "${SENTRY_SILENCE_DIRENV_NOTICE:-}" ] && [ "${log_level}" != "info" ]; then
     cat <<EOF
+
 If you're stuck or have questions, ask in #discuss-dev-tooling.
 
 EOF
diff --git a/Makefile b/Makefile
index b33300850ce3ea..ea2191a3be1a46 100644
--- a/Makefile
+++ b/Makefile
@@ -17,6 +17,7 @@ install-py-dev :
 	@./scripts/do.sh $@
 
 build-platform-assets \
+direnv-help \
 upgrade-pip \
 setup-git-config :
 	@SENTRY_NO_VENV_CHECK=1 ./scripts/do.sh $@
diff --git a/scripts/ensure-venv.sh b/scripts/ensure-venv.sh
index 78c1f10d9e1272..92e33c392e5c67 100755
--- a/scripts/ensure-venv.sh
+++ b/scripts/ensure-venv.sh
@@ -13,7 +13,7 @@ reset="$(tput sgr0)"
 
 venv_name=".venv"
 
-die () {
+die() {
     cat <<EOF
 $@
 EOF
@@ -29,13 +29,13 @@ if [[ -n "$VIRTUAL_ENV" ]]; then
     # unless you explicitely set an environment variable
     if [[ "$minor" -gt 6 ]]; then
         if [[ -n "$SENTRY_PYTHON_VERSION" ]]; then
-            cat << EOF
+            cat <<EOF
 ${yellow}${bold}
 You have explicitly set a non-recommended Python version (${SENTRY_PYTHON_VERSION}). You're on your own.
 ${reset}
 EOF
         else
-            cat << EOF
+            cat <<EOF
 ${red}${bold}
 ERROR! You are running a virtualenv with a Python version different than 3.6
 We recommend you start with a fresh virtualenv or to set the variable SENTRY_PYTHON_VERSION
diff --git a/scripts/lib.sh b/scripts/lib.sh
index 37473dc9acf672..8b4ab5f690f95a 100755
--- a/scripts/lib.sh
+++ b/scripts/lib.sh
@@ -214,3 +214,12 @@ reset-db() {
     create-db
     apply-migrations
 }
+
+direnv-help() {
+    cat >&2 <<EOF
+You can configure the behaviour of direnv by adding the following variables to a .env file:
+
+- SENTRY_DIRENV_NOISY=1: This will allow printing debug messages
+- SENTRY_DEVENV_NO_REPORT=1: Do not report development environment errors to Sentry.io
+EOF
+}


Code review comments:
diff_hunk: @@ -47,19 +48,23 @@ require() {
     command -v "$1" >/dev/null 2>&1
 }
 
-warn() {
+debug() {
+    if [ "${SENTRY_DIRENV_NOISY}" ]; then
+        echo "${@}"
+    fi
+}
+
+info() {
     cat <<EOF
-${yellow}${bold}direnv: ${@}${reset}
path: .envrc
text: @author:
I'm removing `direnv: ` from all lines.

diff_hunk: @@ -47,19 +48,23 @@ require() {
     command -v "$1" >/dev/null 2>&1
 }
 
-warn() {
+debug() {
+    if [ "${SENTRY_DIRENV_NOISY}" ]; then
path: .envrc
text: @author:
This is a very opinionated decision and I understand there might be many folks who would prefer the original info statements by default. If someone wants a noisy output they can opt-in.

My ultimate preference would be to delete all debug/info statements unless necessary.

@user1:
What about `SENTRY_DIRENV_DEBUG` instead?