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

PEP-518 support: configure bandit via pyproject.toml #401

Merged
merged 27 commits into from Aug 24, 2021

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Oct 8, 2018

Thank you for cool lib.

Added support for configuring via pyproject.toml (tool.bandit section). Closes #212, because now all projects have moved from setup.cfg to pyproject.toml.

  1. Parse files the end with .toml as toml and get the tool.bandit section.
  2. Tests included (just inherited from YAML config tests).
  3. Documentation updated, config example included into docs too.

Close #550 as well

doc/source/config.rst Outdated Show resolved Hide resolved


class TestTomlConfig(TestConfigCompat):
sample = textwrap.dedent("""
Copy link
Member

Choose a reason for hiding this comment

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

This block would be good as a fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't fixture. I want to run same test cases twice for two different configs. Fixture, contrariwise, allows you to reuse same set ups in different test cases. Inheritance works perfect in this case.

test-requirements.txt Outdated Show resolved Hide resolved
@orsinium
Copy link
Contributor Author

orsinium commented Oct 9, 2018

2 from 3 review points applied. Thank you for good review :)

@orsinium
Copy link
Contributor Author

orsinium commented Nov 5, 2018

@ericwb, what about this PR? Does you have any other requests?

@orsinium
Copy link
Contributor Author

@ericwb, could you review it again, please? It's ready to merge, I hope. :)

Copy link

@rooterkyberian rooterkyberian left a comment

Choose a reason for hiding this comment

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

I like how this is implemented using the same interface as YAML config 👍

@orsinium
Copy link
Contributor Author

orsinium commented Mar 2, 2019

Thank you ❤️ Could you, please, poke the code owner to finally merge it?

@rooterkyberian
Copy link

@orsinium sadly I don't have any more poking prowess than you, but what I can say that I'm also interested in this features, especially as it possibly may (or may not) affect #318 . That said this eature seems to stand on its own and is worth merging IMO.

you will have to update the PR as there already some file conflicts
thank you for you hard work:)

@ericwb do you think its ready for merging? (after resolving conflicts)

@ericwb
Copy link
Member

ericwb commented Mar 4, 2019

I'm reluctant to add another way to process configuration which adds a new run-time dependency (toml).

@orsinium
Copy link
Contributor Author

orsinium commented Mar 4, 2019

Why? As you said, this is runtime dependency. Nothing changes for users who prefer old format. But support of PEP-518 will be very helpful for most of other people. You already have at least two issues about it.

@rooterkyberian
Copy link

The requirements "issue" can be mitigated by making toml "extra" requirement (https://docs.openstack.org/pbr/3.1.0/index.html#extra-requirements) , and changing the code that on failed toml import it just doesn't attempt to read pyproject.toml

@orsinium
Copy link
Contributor Author

orsinium commented Mar 4, 2019

  1. Conflicts resolved
  2. Extras for yaml and toml added.

setup.py Outdated Show resolved Hide resolved
@bittner
Copy link
Contributor

bittner commented Mar 14, 2019

Personally, I'd prefer to include a [bandit] section in my tox.ini file.

There is a --ini option in the CLI, which qualifies for this feature. Though, auto-discovering that section in a tox.ini present in a project running Bandit (as do other tools like pytest, flake8, behave, etc.) may be helpful to further reduce clutter.

@mverteuil
Copy link

mverteuil commented May 2, 2019

@bittner tox.ini is moving into pyproject.toml, as is everything else python configuration related moving forward. At present, my tox configuration is located in my pyproject.toml as a string, which would make bandit reading from the tox.ini from the pyproject.toml a nasty prospect. I believe this is the real motivation behind all of us moving to the pyproject.toml world, since setup.cfg was previously the thing where you could put almost everything.

I would really appreciate this being merged and helping set the pace for other projects, as it's frustrating that so many projects are just sitting with their pyproject work in a PR holding pattern.

EDIT: Maybe I'm wrong, based on the discussion on the PR in the Flake8 repository, we who thought pyproject.toml was for all tools may have misunderstood the PEP's intention. Now I can understand where the holding comes from, because it's unclear if we should move forward with this or not.

@bittner
Copy link
Contributor

bittner commented May 3, 2019

I fear my comment from above is unclear: I'm not against a [tool.bandit] section in pyproject.toml. I'd rather read settings for Bandit from tox.ini, too. If this make everyone happy we can give pyproject.toml a higher precedence, i.e. search through

  1. .bandit
  2. pyproject.toml
  3. tox.ini

... when you find the first match then stop searching and use that configuration.

P.S.: I find myself in trouble trying to find the documentation on which files Bandit looks for for the INI-style configuration. I know it was somewhere. I only find the config chapter, but that's on the YAML. The docs really need an overhaul... 😟 (see issue #396)

@orsinium
Copy link
Contributor Author

orsinium commented May 3, 2019

@bittner, my PR doesn't affect config lookup at all. In this realization you have to explicitly specify path to the pyproject.toml config to read it. Config lookup can be changed in a separated PR after (and if) this will be merged

@orsinium orsinium requested a review from ericwb October 16, 2019 18:53
@lietu
Copy link

lietu commented Nov 3, 2019

So .. is this never getting merged?

@ericwb
Copy link
Member

ericwb commented Nov 20, 2019

Closes #401

@kamyar
Copy link

kamyar commented Sep 22, 2020

Hi, also looking forward for this PR to get merged.
I am also happy to help any way I can.

@dmyersturnbull
Copy link

@orsinium @rooterkyberian @ericwb @lukehinds What's happening with this PR? Anything I can do?

@orsinium
Copy link
Contributor Author

orsinium commented Jan 5, 2021

What's happening with this PR? Anything I can do?

Nothing's happening from my side. I suppose it only awaits merging. The only thing I can do is to keep the branch up-to-date with the target branch, I guess.

Copy link
Member

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

I am OK with this, but will defer to @ericwb who had some comments in last review.

@pawelmarkowski
Copy link

hello, any updates here? It would be great to see that feature in official release.

@SaschaKrug
Copy link

Would love to see this PR being merged as well

@hnykda
Copy link

hnykda commented Feb 10, 2021

Yeah, same here, looks good?

@sigmavirus24
Copy link
Member

It doesn't look good as there are changes requested by a maintainer.

@SaschaKrug
Copy link

It doesn't look good as there are changes requested by a maintainer.

From what I understand, @ericwb requested to change a block into a fixture and @orsinium answered, why the current solution is fine. That blocked the merge and it was not resolved. Later @ericwb said he was reluctant about the merge, because another dependency would be added, which ended in a new commit to improve this handling.

My questions would be if the fixture and dependency topics are still blocking topics @ericwb? What can be done to resolve them?

We would really love to see this feature being added to improve our configuration setup, because bandit is currently the only one in our stack not supporting pyproject.toml

doc/source/config.rst Outdated Show resolved Hide resolved
doc/source/config.rst Outdated Show resolved Hide resolved
orsinium and others added 2 commits February 16, 2021 11:33
Co-authored-by: Lionel Bersee <lionel1232@gmail.com>
Co-authored-by: Lionel Bersee <lionel1232@gmail.com>
@PyCQA PyCQA locked and limited conversation to collaborators Feb 16, 2021
Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

One small change requested and we can merge this. Thanks for your patience.

doc/source/config.rst Outdated Show resolved Hide resolved
Co-authored-by: Eric Brown <ericwb@users.noreply.github.com>
doc/source/config.rst Outdated Show resolved Hide resolved
@ericwb ericwb enabled auto-merge (squash) August 24, 2021 02:15
Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

LGTM

@ericwb ericwb merged commit 44f5c41 into PyCQA:master Aug 24, 2021
@orsinium orsinium deleted the toml branch August 24, 2021 07:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for pyproject.toml as config file format Add config via setup.cfg