Skip to content

Conversation

reinvantveer
Copy link
Contributor

Description of the change:
Use Pipenv - the Python packaging authority recommended way - as a dependency resolution managemente system and vulnerability checker to install Python dependencies.

This PR is part of #4237 to work towards a more user-friendly way of doing reproducible local builds

Motivation for the change:
Python package dependency is best managed using a fit-for-purpose package manager that checks for dependency conflicts, separates wanted dependencies from required subdependencies and that is able to check for vulnerabilities. The proposed solution is fully backwards compatible.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
…lity checks

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
@reinvantveer
Copy link
Contributor Author

#4477 (comment) was accepted here

@reinvantveer
Copy link
Contributor Author

I guess 5 commits in a PR is better than 380 😆

Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

@reinvantveer wdyt about pip freeze-ing all deps, to avoid issues like this?

@reinvantveer
Copy link
Contributor Author

@reinvantveer wdyt about pip freeze-ing all deps, to avoid issues like this?

@estroz 👍 This question couldn't have come at a better time. This PR already solves this, in https://github.com/operator-framework/operator-sdk/pull/4494/files#diff-82452578a3e817b3b4dc5c154471041589c4b535216c59dbd8982bc18e2f8d32R125 and using pipenv install --deploy, which will only install the versions from Pipfile.lock

So in short: yes I think pinning is good here and pipenv provides a solid method to implement this.

@reinvantveer
Copy link
Contributor Author

@estroz I'd consider pinning cryptography in the Pipfile, but only if future versions of the package are expected to break: looks like 3.4.4 still requires a Rust compiler. But instead of pinning, we could also consider adding (and uninstalling) the Rust compiler to the Dockerfile. It just wouldn't help things along in having a local (i.e. non-dockerized) environment, with an extra compiler added to the mix. I guess it's up to you to choose either way. Would you rather

  • pin the version in the Pipfile to prevent pipenv update-ing to cryptography versions requiring a Rust compiler or
  • add the Rust compiler to the Dockerfile recipe to be able to follow along with updates on cryptography
    ?

@estroz
Copy link
Member

estroz commented Feb 13, 2021

@reinvantveer I totally missed the lock file, awesome!

add the Rust compiler to the Dockerfile recipe

I’m in favor of this option.

@reinvantveer
Copy link
Contributor Author

@reinvantveer I totally missed the lock file, awesome!

add the Rust compiler to the Dockerfile recipe

I’m in favor of this option.

@estroz Cool. I do some Rust programming in my spare time and it's awesome. If it's OK with you I'd like to process this PR first before diving into the Rust compiler to upgrade the cryptography package. This PR is pretty much self-contained (with the whole pinning business through the lockfile) so it should be the first next step I think.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2021
@estroz estroz merged commit 4c0a60d into operator-framework:master Feb 13, 2021
@reinvantveer reinvantveer deleted the pipenv branch February 13, 2021 19:55
@varshaprasad96
Copy link
Member

/cherry-pick v1.4.x

@openshift-cherrypick-robot

@varshaprasad96: #4494 failed to apply on top of branch "v1.4.x":

Applying: add changelog file
Applying: use pipenv install to manage python dependency installs and vulnerability checks
Using index info to reconstruct a base tree...
M	images/ansible-operator/Dockerfile
Falling back to patching base and 3-way merge...
Auto-merging images/ansible-operator/Dockerfile
CONFLICT (content): Merge conflict in images/ansible-operator/Dockerfile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 use pipenv install to manage python dependency installs and vulnerability checks
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v1.4.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@estroz
Copy link
Member

estroz commented Feb 15, 2021

@varshaprasad96 you can just make the bump on this line in a new PR.

@estroz
Copy link
Member

estroz commented Feb 15, 2021

@reinvantveer this PR might've been merged prematurely. Builds for s390x, ppc64le, arm64 are broken due to gcc (and other deps) not being installed, but during some testing I also discovered that the Pipfile.lock's hashes don't resolve on s390x builds (at least) and I can't debug the issue. I'd like to revert this PR until we can test builds on all platforms using Github Actions for each PR since we're moving away from Travis soon. This is something I'm working on right now using docker buildx.

estroz added a commit to estroz/operator-sdk that referenced this pull request Feb 15, 2021
@estroz estroz mentioned this pull request Feb 15, 2021
estroz added a commit to estroz/operator-sdk that referenced this pull request Feb 15, 2021
This reverts commit 4c0a60d.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
estroz pushed a commit that referenced this pull request Feb 15, 2021
This reverts commit 4c0a60d.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
reinvantveer added a commit to reinvantveer/operator-sdk that referenced this pull request Feb 16, 2021
retry of operator-framework#4538 and operator-framework#4494

Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer added a commit to reinvantveer/operator-sdk that referenced this pull request Feb 28, 2021
retry of operator-framework#4538 and operator-framework#4494

Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer added a commit to reinvantveer/operator-sdk that referenced this pull request Mar 5, 2021
retry of operator-framework#4538 and operator-framework#4494

Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer added a commit to reinvantveer/operator-sdk that referenced this pull request Mar 7, 2021
retry of operator-framework#4538 and operator-framework#4494

Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer added a commit to reinvantveer/operator-sdk that referenced this pull request Mar 11, 2021
retry of operator-framework#4538 and operator-framework#4494

Signed-off-by: reinvantveer <reinvantveer@gmail.com>
@estroz estroz mentioned this pull request Apr 13, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants