We should agree on some python3 styles and ensure that the CI checks in the puppet repo implement the agreed styles.
To start things I have heard one complaint related to W504 so that would be the first one to consider
If we really hate theses rules, we can rollback https://gerrit.wikimedia.org/r/c/operations/puppet/+/510613 until we have consensus
we should potentially also refresh discussions on black. potentially with a CI hook to automaticity reformat the code via a pre-commit hook
I will float the opinion that while I may have many opinions on code style, bikeshedding between reasonable options for a shared standard is a waste. If there's a standard upstream/outside-world set of common style rules for python3, we should just adopt them and be done with it.
I don't like the idea of auto-reformatting code in a commit hook, though. We can make sure the formatting tool is easy to run for when one wants to, but I think forcing it as part of the commit process is a bit heavy-handed if CI's going to call out the same things.
I think the best way is to have it easily integrated in some form in the local workflow in our dev envs, so that when you tests locally they passes and when you commit you commit already the formatted version. Then CI is just ensuring that the code is already formatted according to the tool.
Otherwise the resulting workflow would be annoying: make a patch, send it to Gerrit, get V-1 from Jenkins, check the output, either run the tool manually locally or fix manually the format issues, send a second PS.
Change 553487 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] CI - taskgen: add black tests for python2 and python3 files
The .git directory is not versioned, therefore a git hook is by nature, opt-in. If we go down the route of using black i would suggest creating a git hook that users can install and if/when CI fails we include a message that points users to instructions on how to install the hook.
On a more practice created a patch set to see what this would look like on the CI side and the first issue we would need to fix is the python version. black requires python3.6+ however the jenkins containers are still running stretch i.e. python3.5. We would also need to take this into account for an commit hooks
@jbond on the CI instances you have 3.4, 3.5, 3.6 and 3.7 available although the system one is 3.5. Faidon did the packaging a while ago and if you see the CI jobs of many repos they run all environments from tox.
Change 553502 had a related patch set uploaded (by Jbond; owner: John Bond):
[integration/config@master] operations-puppet: add python3-{4,6,7} to image
Change 553502 merged by jenkins-bot:
[integration/config@master] operations-puppet: add python3-{4,6,7} to image
Mentioned in SAL (#wikimedia-releng) [2019-12-03T10:45:25Z] <hashar> Building releng/operations-puppet to upgrade tox and ship python3.4 3.6 and 3.7 # T239334
Change 554263 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Update tox, add python3.6 to operations/puppet
Change 554263 merged by jenkins-bot:
[integration/config@master] Update tox, add python3.6 to operations/puppet
Change 554270 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] docker: fix cache owner in operations-puppet
Change 554270 merged by jenkins-bot:
[integration/config@master] docker: fix cache owner in operations-puppet
Change 554271 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Update puppet job to fix pip cache ownership
Change 554271 merged by jenkins-bot:
[integration/config@master] Update puppet job to fix pip cache ownership
The CR to black is now working and you can see what changes are highlighted in this CI console output
Line length needs to be tweaked to conform with our puppet settings for flake8 I guess.
I'm pretty sure the line length configured in the tox.ini file applies to both the python2 and python3 test. Although why this was changed to 100 and if this is correct pre-dates my time and there is not much info in the original commit :)
however if you are seeing different result with python 2 and python3 then that is a bug
Edit: Thinking again you probably meant that black needed to be updated to use 100 char line length (will update)
Here is another update with line length set to 100
Edit: Thinking again you probably meant that black needed to be updated to use 100 char line length (will update)
Here is another update with line length set to 100
And here is one without string normalisation