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

Safety 2 use without a subprocess #5476

Conversation

yeisonvargasf
Copy link
Contributor

@yeisonvargasf yeisonvargasf commented Nov 16, 2022

The issue

  • The pipenv new Safety implementation is using a subprocess.

  • There isn't a non-verbose output.

This pull request is related to #5218

The fix

  • pipenv check uses Safety directly, replacing the Safety implementation with a subprocess.

  • Added a new minimal output, a non-verbose output.

@oz123
Copy link
Contributor

oz123 commented Nov 18, 2022

@yeisonvargasf your vendoring stage is failing because you started off with an outdated branch.

Please cherry-pick this commit onto your branch;

6f88313

@oz123 oz123 requested review from oz123 and matteius and removed request for oz123 November 18, 2022 09:39
@oz123
Copy link
Contributor

oz123 commented Nov 18, 2022

@yeisonvargasf can you please rebase your changes on top of the current main branch?

@yeisonvargasf yeisonvargasf force-pushed the safety2-without-subprocess branch from d529381 to 602bd0c Compare November 19, 2022 00:21
@yeisonvargasf
Copy link
Contributor Author

Thanks, @oz123! I rebased my changes on top of the current main branch.

The PR base branch is the @matteius branch; do we need to change the base branch to main?

On the other hand, running the tests in my local environment fails because Safety detects a vulnerability in the tested environment. Could you help to review that?

After the test issue is solved, we'll need vendoring the latest Safety version that PyUp will release next week; it fixes an issue related to ruamel and its use in pipenv check.

@oz123
Copy link
Contributor

oz123 commented Nov 19, 2022

@yeisonvargasf we don't have base branch, it's just main. Currently, your PR isn't mergeable. Please rebase again.

@yeisonvargasf yeisonvargasf changed the base branch from vendor-latest-safety2 to main November 19, 2022 13:42
@matteius
Copy link
Member

@yeisonvargasf I don't know if you could go back to the way this branch was before the attempted rebase? It probably would have worked smoother to have merged main branch changes into this fork/branch instead of how this rebase was done.

@matteius
Copy link
Member

I can update my branch with the latest main changes now if that helps, you can continue to target that branch, but right now the commit history here seems corrupted from the rebasing, perhaps you can cherry-pick your original commits to a new branch off my updated branch?

@matteius
Copy link
Member

I have pushed the updated changes from main to my branch vendor-latest-safety2

@yeisonvargasf yeisonvargasf changed the base branch from main to vendor-latest-safety2 November 19, 2022 14:03
@yeisonvargasf
Copy link
Contributor Author

I think the same, so sure, @matteius; I'll create a new branch and make a new PR with the vendor-latest-safety2 as a target.

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.