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

Consolidate pipenv settings to use get_from_env #5451

Merged
merged 10 commits into from
Nov 4, 2022

Conversation

micahjsmith
Copy link
Contributor

@micahjsmith micahjsmith commented Nov 2, 2022

The issue

See #5407

  • some pipenv settings are insufficiently documented, especially in terms of how to disable them
  • implementation of pipenv settings detection is inconsistent, even though pipenv.environments.get_from_env exists as a pretty hardened utility

The fix

  • refactor all relevant settings to use get_from_env, which more explicitly allows settings to be disabled
    • add default kwarg to get_from_env
  • update docs to indicate how to explicitly disable a setting

Other changes

  • Fix rst in contributing guide
  • Refactor out is_env_truthy

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@micahjsmith micahjsmith marked this pull request as ready for review November 2, 2022 02:42
@micahjsmith
Copy link
Contributor Author

@oz123 I'm getting this failed test case below. is it possible the test case is broken because a new vulnerability for wheel has been reported (51499)? a bit of a funky test case because it seems to assume that there will never be a new vulnerability in any of the installed packages. unless I am misinterpreting what is happening here

_________________________________________________________________________________________________________________________________________________________________________ test_pipenv_check __________________________________________________________________________________________________________________________________________________________________________

pipenv_instance_private_pypi = functools.partial(<class 'tests.integration.conftest._PipenvInstance'>, capfd=<_pytest.capture.CaptureFixture object at 0x1162d8190>)

    @pytest.mark.cli
    @pytest.mark.needs_internet(reason='required by check')
    @flaky
    def test_pipenv_check(pipenv_instance_private_pypi):
        with pipenv_instance_private_pypi() as p:
            c = p.pipenv('install pyyaml')
            assert c.returncode == 0
            c = p.pipenv('check')
            assert c.returncode != 0
            assert 'pyyaml' in c.stdout
            c = p.pipenv('uninstall pyyaml')
            assert c.returncode == 0
            c = p.pipenv('install six')
            assert c.returncode == 0
            c = p.pipenv('check --ignore 35015')
>           assert c.returncode == 0
E           assert 1 == 0
E            +  where 1 = <Result SystemExit(1)>.returncode

tests/integration/test_cli.py:156: AssertionError
------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Captured stdout call ------------------------------------------------------------------------------------------------------------------------------------------------------------------------
$ pipenv check --ignore 35015
Checking PEP 508 requirements...
Passed!
Checking installed package safety...
51499: wheel <0.38.0 resolved (0.37.1 installed)!
Wheel 0.38.0 fixes a potential DoS attack via the 'WHEEL_INFO_RE' regular expression.


Command failed...
------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Captured stderr call ------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Notice: Ignoring CVE(s) 35015

Traceback (most recent call last):
  File "/Users/micahsmith/workspace/pipenv/.venv/lib/python3.10/site-packages/click/testing.py", line 408, in invoke
    return_value = cli.main(args=args or (), prog_name=prog_name, **extra)
  File "/Users/micahsmith/workspace/pipenv/pipenv/cli/options.py", line 57, in main
    return super().main(*args, **kwargs, windows_expand_args=False)
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/decorators.py", line 84, in new_func
    return ctx.invoke(f, obj, *args, **kwargs)
  File "/Users/micahsmith/workspace/pipenv/pipenv/vendor/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/micahsmith/workspace/pipenv/pipenv/cli/command.py", line 480, in check
    do_check(
  File "/Users/micahsmith/workspace/pipenv/pipenv/core.py", line 2921, in do_check
    sys.exit(1)
SystemExit: 1

@micahjsmith
Copy link
Contributor Author

went ahead and updated the test case to ignore the new vulnerability, tests pass locally now

@oz123
Copy link
Contributor

oz123 commented Nov 2, 2022

@micahjsmith see my PR #5450 there is a fix for that wheel issue . Thank you for putting up the time for this PR!

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.

2 participants