Workflow; support, but don't require, pyenv

[petemounce]

Travis automatically has pyenv + pyenv-virtualenv support, so I expect/hope this to be lovely. No, it doesn't. Nuts. Still, this enables some workstation pinning.

pyenv-virtualenv will, when the shell has had eval $(pyenv virtualenv-init -) done, automatically enter a virtualenv if a .python-version file is found when cd'ing into a directory.

https://github.com/pyenv/pyenv-virtualenv

Changes

Verification

[DoomGerbil]

Am I misreading it, or does this set $repo_root to dev/ within the repo? That seems misleading at best.

[ca-johnson]

Not sure why the existing line below readonly root="${BASH_SOURCE%/*}/.." was deleted - this seems more readable.

[petemounce]

I need the variable within the error message on line 7.

re: readability - settle in... https://stackoverflow.com/questions/59895/how-to-get-the-source-directory-of-a-bash-script-from-within-the-script-itself is a joy. 🤯

[DoomGerbil]

@petemounce And I notice that in that case, it's referencing .python-version at the root of the repo as $repo_root/../.python-version.

That's my problem here - that repo_root isn't actually the repo root. It's basically repo_root=ACTUAL_REPO_ROOT/dev which IMO is definitely not correct.

[Helcaraxan]

+1 on @DoomGerbil's point here. This line is not setting the right directory. Am suggestion the cleaned-up version.

Suggested change

	readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)"
	readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)"

The pipe to /dev/null is not required as the cd should not print anything if successful and we bail-out anyway if there's an error (due to set -e above) and we would like to see the output in that case.

[DoomGerbil]

I'm unsure about the value we're setting for the repo_root here. Assuming I didn't misread the one-liner, I would prefer that repo_root actually reference the repo root, and then the rest of the paths using it be relative to that, rather than setting that var to /dev within the repo and then acting relative to that.

Otherwise LGTM

[improbable-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DoomGerbil, PaulSonOfLars
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[DoomGerbil]

Needs approval from an approver in each of these files:

• OWNERS

FYI @petemounce we have no OWNERS in this repo. We probably wanna address that at some point, though not super urgent.

[Helcaraxan]

+1 on @DoomGerbil's point here. This line is not setting the right directory. Am suggestion the cleaned-up version.

Suggested change

	readonly repo_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)"
	readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)"

The pipe to /dev/null is not required as the cd should not print anything if successful and we bail-out anyway if there's an error (due to set -e above) and we would like to see the output in that case.

[Helcaraxan]

Suggested change

	echo >&2 "I require python3 but it's not installed. Officially supported version is $(cat "${repo_root}/../.python-version")"
	echo >&2 "I require python3 but it's not installed. Officially supported version is $(cat "${repo_root}/.python-version")"

Fixing this in relationship to the corrected repo_root above in my suggestion, and to make it consistent with the other uses of ${repo_root} below in this script.