Skip to content
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.

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

refactor: use pipenv to manage dependencies #150

Merged
merged 1 commit into from
May 18, 2022
Merged

Conversation

4141done
Copy link
Contributor

@4141done 4141done commented May 16, 2022

What

Using pip and requirements.txt led to issues where dependent library versions were
unpredictable and led to errors in some deployment environments.

This will also allow us to use semantic version properly without needing to explicitly
pin dependencies at certain versions but perform updates to libraries intentionally.

Proposed changes

  1. Install and use Pipenv which will generate a Pipfile.lock detailing the exact versions of dependencies and their dependencies. This will help us have more reproducible builds since each dependency and subdependency will be installed at the exact version it was tested with.
  2. Changed the code that determines the pulumi cli version to use the output of pip rather than reading from the requirements file (since that's not an exact version).

Pipenv Usage

pipenv is compatible with any commands that can be given to pip. There are some niceties that are good to know too:

graph

Prints the dependency graph with the Pipfile specs and the actual version resolved to.

$ pipenv graph
awscli==1.22.101
  - botocore [required: ==1.24.46, installed: 1.24.46]
    - jmespath [required: >=0.7.1,<2.0.0, installed: 1.0.0]
    - python-dateutil [required: >=2.1,<3.0.0, installed: 2.8.2]
      - six [required: >=1.5, installed: 1.16.0]
    - urllib3 [required: >=1.25.4,<1.27, installed: 1.26.9]
  - colorama [required: >=0.2.5,<0.4.4, installed: 0.4.3]
  - docutils [required: >=0.10,<0.16, installed: 0.15.2]
  - PyYAML [required: >=3.10,<5.5, installed: 5.4.1]
  - rsa [required: >=3.1.2,<4.8, installed: 4.7.2]
    - pyasn1 [required: >=0.1.3, installed: 0.4.8]
  - s3transfer [required: >=0.5.0,<0.6.0, installed: 0.5.2]
    - botocore [required: >=1.12.36,<2.0a.0, installed: 1.24.46]
      - jmespath [required: >=0.7.1,<2.0.0, installed: 1.0.0]
      - python-dateutil [required: >=2.1,<3.0.0, installed: 2.8.2]
        - six [required: >=1.5, installed: 1.16.0]
      - urllib3 [required: >=1.25.4,<1.27, installed: 1.26.9]
fart==0.1.5
  - pyperclip [required: Any, installed: 1.8.2]
kic-pulumi-utils==1.0.0.post37+git.fb325311.dirty
  - GitPython [required: >=3.1.18,<3.2.0, installed: 3.1.27]
    - gitdb [required: >=4.0.1,<5, installed: 4.0.9]
      - smmap [required: >=3.0.1,<6, installed: 5.0.0]
  - passlib [required: >=1.7.4,<2.0.0, installed: 1.7.4]
  - pyyaml [required: >=5.3.1,<6.0, installed: 5.4.1]

... output truncated

Figuring out updates

You can use pipenv update --outdated to display a list of packages that have newer versions in the upstream.
From there, you can choose to update them one by one with pipenv update <package name> which will update the package according to the rules specified in the Pipfile. You may need to change the specification in the Pipfile to get the latest version.

https://pipenv-fork.readthedocs.io/en/latest/basics.html#example-pipenv-upgrade-workflow

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

# Install certain utility packages like `nodeenv` and `wheel` that aid
# in the installation of other build tools and dependencies
# required by the other python packages.
PIPENV_VERBOSITY=-1 PIPENV_PIPFILE="${script_dir}/../pulumi/python/Pipfile" pipenv install --dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

annotation:
Here we are:

  1. Silencing an informational message telling us that pipenv is going to use the current venv with PIPENV_VERBOSITY=-1
  2. Pointing pipenv at our Pipfile which is not in the current dir with PIPENV_PIPFILE
  3. Only installing dev depenencies which are currently nodeenv and wheel

@@ -195,12 +194,15 @@ else
fi

# Install general package requirements
pip3 install --requirement "${script_dir}/../pulumi/python/requirements.txt"
PIPENV_VERBOSITY=-1 PIPENV_PIPFILE="${script_dir}/../pulumi/python/Pipfile" pipenv install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

annotation:
Here we are:

  1. Silencing an informational message telling us that pipenv is going to use the current venv with PIPENV_VERBOSITY=-1
  2. Pointing pipenv at our Pipfile which is not in the current dir with PIPENV_PIPFILE
  3. Installing all packages

Comment on lines +200 to +205
pip3 install "${script_dir}/../pulumi/python/utility/kic-pulumi-utils"

rm -rf "${script_dir}/../pulumi/python/utility/kic-pulumi-utils/.eggs" \
"${script_dir}/../pulumi/python/utility/kic-pulumi-utils/build" \
"${script_dir}/../pulumi/python/utility/kic-pulumi-utils/kic_pulumi_utils.egg-info"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

annotation:
Removing the && prevents an issue where if the pip install does not work the script would not fail. The rm operation should not fail either way.

echo "Downloading Pulumi CLI into virtual environment"
PULUMI_VERSION="$(grep '^pulumi~=.*$' "${script_dir}/../pulumi/python/requirements.txt" | cut -d '=' -f2 || true)"
PULUMI_VERSION="$(pip list | grep 'pulumi ' | sed -nre 's/^[^0-9]*(([0-9]+\.)*[0-9]+).*/\1/p')"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

annotation:
Here we read the pulumi version from pip to get the version that was actually installed.

Copy link
Collaborator

@dekobon dekobon left a comment

Choose a reason for hiding this comment

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

This is a great change! 🍥

pip3 install --upgrade pip
pip install pipenv
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use pip3 here to bring it in harmony with the above line.

@4141done 4141done marked this pull request as ready for review May 17, 2022 16:31
@qdzlug
Copy link
Contributor

qdzlug commented May 18, 2022

@4141done - this looks good to me; out of an abundance of caution I just kicked off all the deploy tests, but once they are clear I'm happy to have this merged whenever you are ready (my PR for #152 only touches the Jenkinsfile so it's going to be fine with yours).

@qdzlug qdzlug self-requested a review May 18, 2022 15:53
Copy link
Contributor

@qdzlug qdzlug left a comment

Choose a reason for hiding this comment

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

Looks good to me...

Using pip and `requirements.txt` led to issues where dependent library versions were
unpredictable and led to errors in some deployment environments.

This will also allow us to use semantic version properly without needing to explicitly
pin dependencies at certain versions but perform updates to libraries intentionally.
@qdzlug qdzlug merged commit 8c9a299 into nginxinc:master May 18, 2022
@qdzlug qdzlug mentioned this pull request May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants