Limit the maximum block depth #3602

Ruchip16 · 2023-07-04T11:01:56Z

Since inside tasks there can be nested blocks so this PR limits the depth of the block by a certain limit which helps to maintain the readability of the playbooks.

fixes: #2173

Ruchip16 · 2023-07-06T07:30:33Z

hey @ssbarnea @ajinkyau can you review this please?

examples/playbooks/rule-complexity-fail.yml

src/ansiblelint/rules/complexity.py

- fix counting of depth - rename rule id to 'complexity[nesting]' - add missing documentation - add missing configuration schema - include configurable example in sample config file

ssbarnea · 2023-08-21T17:38:56Z

hey @ssbarnea @ajinkyau can you review this please?

Please check the last commit and see all the extra changes I had to do there. The original code was not really working and the tests were failing to check for the behavior. Still, it was not too hard to fix.

Outdated, please check again

Ruchip16 · 2023-08-21T17:52:03Z

hey @ssbarnea @ajinkyau can you review this please?

Please check the last commit and see all the extra changes I had to do there. The original code was not really working and the tests were failing to check for the behavior. Still, it was not too hard to fix.

the main thing i got wrong and really confused was with the example of the block, i although discussed that once with ajinkya, but nevertheless, apologies for the extra work and would contribute more to get a good hang of it!

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ansible-lint](https://github.com/ansible/ansible-lint) ([changelog](https://github.com/ansible/ansible-lint/releases)) | minor | `==6.17.2` -> `==6.18.0` | --- ### Release Notes <details> <summary>ansible/ansible-lint (ansible-lint)</summary> ### [`v6.18.0`](https://github.com/ansible/ansible-lint/releases/tag/v6.18.0) [Compare Source](ansible/ansible-lint@v6.17.2...v6.18.0) #### Minor Changes - Limit the maximum block depth ([#3602](ansible/ansible-lint#3602)) [@Ruchip16](https://github.com/Ruchip16) - Transform functionality for command_instead_of_shell ([#3675](ansible/ansible-lint#3675)) [@ajinkyau](https://github.com/ajinkyau) - Add rule to check the number of tasks ([#3156](ansible/ansible-lint#3156)) [@Ruchip16](https://github.com/Ruchip16) #### Bugfixes - Clarify loop-var-prefix rule and code snippet ([#3642](ansible/ansible-lint#3642)) [@schwarmco](https://github.com/schwarmco) - Update `version_added` for complexity rule ([#3623](ansible/ansible-lint#3623)) [@ajinkyau](https://github.com/ajinkyau) - Ensure that single space between tasks is preserved when using --write ([#3641](ansible/ansible-lint#3641)) [@shatakshiiii](https://github.com/shatakshiiii) - Update ansible-compat used for testing ([#3664](ansible/ansible-lint#3664)) [@ssbarnea](https://github.com/ssbarnea) - Document `yaml[line-length]` rule ([#3653](ansible/ansible-lint#3653)) [@shatakshiiii](https://github.com/shatakshiiii) - Prevent use of spdx-tools 0.8.0 due to breaking changes ([#3649](ansible/ansible-lint#3649)) [@ssbarnea](https://github.com/ssbarnea) - fixes dead marketplace link ([#3631](ansible/ansible-lint#3631)) [@wookietreiber](https://github.com/wookietreiber) - Improve profile information on summary line ([#3637](ansible/ansible-lint#3637)) [@ziegenberg](https://github.com/ziegenberg) - command-instead-of-module: allow `git rev-parse` ([#3610](ansible/ansible-lint#3610)) [@JohnVillalovos](https://github.com/JohnVillalovos) - Include filepaths starting from $HOME in lintables ([#3621](ansible/ansible-lint#3621)) [@shatakshiiii](https://github.com/shatakshiiii) - Update \_mockings.py to fix bug created in [#3390](ansible/ansible-lint#3390) ([#3614](ansible/ansible-lint#3614)) [@karcaw](https://github.com/karcaw) - Allow to set gather_facts as templated boolean ([#3606](ansible/ansible-lint#3606)) [@noonedeadpunk](https://github.com/noonedeadpunk) - Add dependency version check for collection metadata ([#3601](ansible/ansible-lint#3601)) [@ajinkyau](https://github.com/ajinkyau) - Fix installation of dependencies when run as an action ([#3592](ansible/ansible-lint#3592)) [@ssbarnea](https://github.com/ssbarnea) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.home/nrdufour/home-ops/pulls/49 Co-authored-by: Renovate <[email protected]> Co-committed-by: Renovate <[email protected]>

bluikko · 2023-08-28T10:29:53Z

Does this PR really limit maximum block depth, or tasks inside a block?

There seems to be a conflict between the PR name and the help files contained in the PR diff:

PR name talks about "block depth", i.e. nested blocks.
PR diff contains 2 new rule description items which only talk about number of tasks inside block.

Please clarify which one it is, something is wrong here.

More precisely:

`complexity[nesting]` will appear when a block contains too many tasks, by
default that number is 20 but it can be changed inside the configuration file by
defining `max_block_depth` value.

This rule description already has a conflict as well. It talks about number of tasks inside a block, but the rule name talks about "nesting" and the config param name includes "depth", contrary to the description text. It just adds to the confusion around what does this actually do. This is even listed in release notes talking about "maximum block depth", i.e. how many layers in nesting. But this does not match the rule descriptions at all.

Ruchip16 · 2023-08-28T10:47:20Z

limit maximum block depth means that it limits the depth of any block (nested or not) up to a certain number which is currently 20 by default, if the tasks inside a block are > 20 it would raise a warning message, so maximum block depth refers to the depth of the block and nesting is a scenario inside that, hope that clears your confusion, but yes we need to do something if it is creating a confusion

bluikko · 2023-08-28T12:13:36Z

limit maximum block depth means that it limits the depth of any block

It does not, at all. It strengthens my opinion that the words are misused here.
"Depth" refers to how many nested layers there are in my opinion.

More suitable terms would include: "length", "size", etc.

max block depth fix

2073f31

Ruchip16 added the skip-changelog Can be missed from the changelog. label Jul 4, 2023

Ruchip16 self-assigned this Jul 4, 2023

Ruchip16 requested a review from a team as a code owner July 4, 2023 11:01

Ruchip16 requested review from a team, audgirka, priyamsahoo, shatakshiiii and ssbarnea July 4, 2023 11:01

audgirka previously requested changes Jul 6, 2023

View reviewed changes

examples/playbooks/rule-complexity-fail.yml Outdated Show resolved Hide resolved

audgirka reviewed Jul 6, 2023

View reviewed changes

src/ansiblelint/rules/complexity.py Outdated Show resolved Hide resolved

audgirka marked this pull request as draft July 11, 2023 10:21

Ruchip16 added 2 commits August 21, 2023 16:13

review changes

e771102

Merge branch 'main' into max/depth

a9d6440

Ruchip16 force-pushed the max/depth branch from 08c04e1 to a9d6440 Compare August 21, 2023 10:56

Ruchip16 marked this pull request as ready for review August 21, 2023 10:57

Ruchip16 and others added 2 commits August 21, 2023 17:02

test changes

d0197b7

Merge branch 'main' into max/depth

b7a63dd

ssbarnea added enhancement and removed skip-changelog Can be missed from the changelog. labels Aug 21, 2023

Finish implementation

fa2f006

- fix counting of depth - rename rule id to 'complexity[nesting]' - add missing documentation - add missing configuration schema - include configurable example in sample config file

ssbarnea requested review from audgirka and cidrblock August 21, 2023 17:37

cidrblock approved these changes Aug 21, 2023

View reviewed changes

ssbarnea merged commit 385ad82 into ansible:main Aug 21, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit the maximum block depth #3602

Limit the maximum block depth #3602

Ruchip16 commented Jul 4, 2023

Ruchip16 commented Jul 6, 2023 •

edited

Loading

ssbarnea commented Aug 21, 2023

Ruchip16 commented Aug 21, 2023

bluikko commented Aug 28, 2023 •

edited

Loading

Ruchip16 commented Aug 28, 2023 •

edited

Loading

bluikko commented Aug 28, 2023 •

edited

Loading

Limit the maximum block depth #3602

Limit the maximum block depth #3602

Conversation

Ruchip16 commented Jul 4, 2023

Ruchip16 commented Jul 6, 2023 • edited Loading

ssbarnea commented Aug 21, 2023

Ruchip16 commented Aug 21, 2023

bluikko commented Aug 28, 2023 • edited Loading

Ruchip16 commented Aug 28, 2023 • edited Loading

bluikko commented Aug 28, 2023 • edited Loading

Ruchip16 commented Jul 6, 2023 •

edited

Loading

bluikko commented Aug 28, 2023 •

edited

Loading

Ruchip16 commented Aug 28, 2023 •

edited

Loading

bluikko commented Aug 28, 2023 •

edited

Loading