Python3

[hwhesselink]

PR progress checklist (to be filled in by reviewers)

• [ ] Changes to documentation are appropriate (or tick if not required)
• [ ] Changes to tests are appropriate (or tick if not required)
• [ ] Reviews completed

---

What type of PR is this?

Primary type

• [ ] [build] Changes related to the build system
• [ ] [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ ] [ci] Changes to the continuous integration configuration
• [ ] [feat] A new feature
• [x] [fix] A bug fix
• [ ] [perf] A code change that improves performance
• [ ] [refactor] A code change that neither fixes a bug nor adds a feature
• [ ] [revert] A change used to revert a previous commit
• [ ] [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [ ] [docs] Documentation changes
• [ ] [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Python3 dropped iteritems, this PR changes them to items. The only downside to items is that, because it's not a generator, it doesn't handle large dataset well. Given that the changes are for the interfaces and zones files I don't believe that will be an issue.

BTW, the Travis CI check is failing on Wheezy because Debian no longer supports it so the build env. download isn't found.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

• [ ] Updated the README (e.g. Available states).
• [ ] Updated pillar.example.

Testing checklist

• [ ] Included in Kitchen (i.e. under state_top).
• [ ] Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• [ ] Updated the relevant test pillar.

Additional context

[pull-assistant[bot]]

Best reviewed: commit by commit

---

Optimal code review plan

     Merge pull request #1 from saltstack-formulas/master      change {}.iteritems() to {}.items() for python3      change {}.iteritems() to {}.items() for python3      remove Python version dependency      remove Python version dependency

Powered by Pull Assistant. Last update c879b8f ... 0b63373. Read the comment docs.

[hwhesselink]

I just submitted a PR to remove Wheezy from testing.

Agreed, using dictsort is a cleaner solution, I've updated the PR.

[myii]

Haven't checked this PR in any detail but please be aware of the various issues surrounding |dictsort and .items() here:

• https://github.com/saltstack-formulas/bind-formula/issues/146

[hwhesselink]

Hi @myii, the |dictsort change only applies to the zone and interface names. They are always strings, and the sort order doesn't matter (the previous iteritems and items also don't sort), so I don't think there are issues in this case. I've been using the code in production for a while.

Thanks for merging the "Drop wheezy testing" PR. Travis is still failing with that test atm, do I need to do anything to this PR to update the testing?

[myii]

Hi @myii, the |dictsort change only applies to the zone and interface names. They are always strings, and the sort order doesn't matter (the previous iteritems and items also don't sort), so I don't think there are issues in this case. I've been using the code in production for a while.

@hwhesselink That should be absolutely fine, then. I'll just let @vutny confirm and merge this.

Thanks for merging the "Drop wheezy testing" PR. Travis is still failing with that test atm, do I need to do anything to this PR to update the testing?

You're welcome. There are two ways that we could proceed:

1. Usually, the contributor can rebase on the latest merged changes and then force push the PR's branch again -- that would avoid the wheezy failure here.
2. However, since we've already merged that and the other jobs are passing, we can just go ahead and merge this as-is -- they'll pass in the merge commit.

Essentially, don't worry about it in this particular PR but you may need to use option 1 elsewhere.

[myii]

... (the previous iteritems and items also don't sort)

Just a note, as mentioned here, .items() isn't random anymore since around Python 3.6.

[hwhesselink]

@myii, great, I'll wait for the merge.

My original PR actually used .items() but I changed it to use |dictsort based on @vutny's comment earlier in this thread. Both work fine.

After this is merged I'll submit a larger patch to update the formula to work with Shorewall v5.

[myii]

Any final comments, @vutny? Would you mind merging if this is ready to go?

[myii]

Merged, thanks for the PR @hwhesselink and for the review @vutny.