Allow setup-env to specify Python version

michaelsaki commented 5 months ago

🗣 Description

This commit is introducing 3 new flags into the setup-env script. -l or--list-versions will list available Python versions and allow the user to select a version interactively. The second flag -v or --version will allow a user to set the version if installed. The third flag -n or --name will allow a user to specify the name of the virtual environment. (e.g. ./setup-env -v 3.9.6 or ./setup-env --name myVenv)

💭 Motivation and context

This will resolve 153. The setup-env script currently creates the Python virtual environment without specifying a Python version via the command pyenv virtualenv <venv_name>. Because no Python version is specified, pyenv uses the system Python. This pull request will allow a user to specify which Python version they want to use.

🧪 Testing

Automated tests all passed. I tested the changes locally with different Python versions installed on my system.

✅ Pre-approval checklist

[x] This PR has an informative and human-readable title.
[x] Changes are limited to a single goal - eschew scope creep!
[x] All relevant type-of-change labels have been added.
[x] I have read the CONTRIBUTING document.
[x] These code changes follow cisagov code standards.
[x] All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
[x] All new and existing tests pass.

jsf9k commented 5 months ago

I tested this script via

./setup-env
./setup-env -l
./setup-env -v 3.12.1 (Python 3.12.1 is installed on my system)
./setup-env -v 3.13.1 (Python 3.13.1 does not exist and hence it is not installed on my system)

All these cases now appear to function as expected, but it would be good if someone else tested as well since I am not in the habit of using this script - although with these changes I intend to start.

michaelsaki commented 5 months ago

There is one more problem. The original script allows one to specify the name of the virtual environment to be created, but this functionality is broken if you specify either of the new options.

To accomodate this, I suggest changing the option parsing code to use getopt. You can find lots of tutorials for getopt online.

Note that I also unchecked some of the checklist items in the PR description since they have not been done. If they do not need to be done then you can simply remove them.

Thanks for catching this I will get it fixed.

jsf9k commented 5 months ago

There is one more problem. The original script allows one to specify the name of the virtual environment to be created, but this functionality is broken if you specify either of the new options. To accomodate this, I suggest changing the option parsing code to use getopt. You can find lots of tutorials for getopt online. Note that I also unchecked some of the checklist items in the PR description since they have not been done. If they do not need to be done then you can simply remove them.

Thanks for catching this I will get it fixed.

If it's OK with the other members of @cisagov/team-ois, I have no issue with you changing the script to require a CLI switch (like -n/--name) when specifying the name of the Python virtual environment. That might make things a bit easier. I don't think naming the virtual environment explicitly is a feature that gets used very often.

dav3r commented 5 months ago

If it's OK with the other members of @cisagov/team-ois, I have no issue with you changing the script to require a CLI switch (like -n/--name) when specifying the name of the Python virtual environment. That might make things a bit easier. I don't think naming the virtual environment explicitly is a feature that gets used very often.

I'm fine with that since I always use the default name for the virtual environment (the repo name).

jsf9k commented 5 months ago

There is one more problem. The original script allows one to specify the name of the virtual environment to be created, but this functionality is broken if you specify either of the new options. To accomodate this, I suggest changing the option parsing code to use getopt. You can find lots of tutorials for getopt online. Note that I also unchecked some of the checklist items in the PR description since they have not been done. If they do not need to be done then you can simply remove them.

Thanks for catching this I will get it fixed.

Using getopt will probably give you the first bullet here for free.

michaelsaki commented 5 months ago

@jsf9k @dav3r I just made a decent amount of changes from my original script. But I do believe the intended functionality is now present.

Note the long options (--force) are now disabled because the BSD version of getopt doesn't support the long options. The GNU version does but then it would be a dependency requirement with running the ./setup-env script.

Also the -n flag is now present if a user wants to specify what name they want to use for a virtual environment and will default to the current directory if they don't.

I do think some of my changes could be improved upon but I wanted to get what I have working up to review to see if the functionality is what everyone was expecting.

jsf9k commented 4 months ago

Please update the PR description. It mentions the new -l and -v options but not the new -n option.

michaelsaki commented 4 months ago

Please update the PR description. It mentions the new -l and -v options but not the new -n option.

Updated! 😊

cisagov / skeleton-generic