Change Details

We have many scripts that are defined as ERB templates. We often make simple errors when editing them, as there's no tooling for dealing with such a format -- and in fact it's quite hard to write tooling for such, given the format of Puppet catalogs (which elides which template file produced a given generated file). Instead, let's forbid scripts being generated from templates. (And also set up `shellcheck` for .sh files.) ===== original/deprecated follows ===== ~~Ideally CI would automatically expand the templates as best it could*, and then run all the 'usual' checks we'd run for such a language. However, there's several tricky parts of doing that. Having a way to specify (in a puppet spec test?) to run shell/Python checks on a template given certain inputs might be more achievable.~~ ~~We could also make this a wrapper around PCC for manual testing of a few key scripts, to start with, if that was easier. Here's a v0 proof of concept that, given PCC output to expand the templates, yoinks the script out of the post-change catalog and runs it through shellcheck (which identifies an issue that caused problems when the patch was merged):~~ ```% curl -s https://puppet-compiler.wmflabs.org/compiler1003/22927/puppetmaster1002.eqiad.wmnet/change.puppetmaster1002.eqiad.wmnet.pson \ | jq -r '.resources[] | select(.title == "/usr/local/bin/puppet-merge") | .parameters.content' \ | shellcheck - In - line 181: if [ $LABS_PRIVATE -eq 1 -a ${LABS_EXIT} -ne 99]; then ^-- SC1009: The mentioned syntax error was in this if expression. ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks. ^-- SC1020: You need a space before the ]. ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again. ```

We have many scripts that are defined as ERB templates. We often make simple errors when editing them, as there's no tooling for dealing with such a format -- and in fact it's quite hard to write tooling for such, given the format of Puppet catalogs (which elides which template file produced a given generated file). Instead, let's forbid scripts being generated from templates. (And also set up `shellcheck` for .sh files.) ===== original/deprecated follows ===== ~~Ideally CI would automatically expand the templates as best it could*, and then run all the 'usual' checks we'd run for such a language. However, there's several tricky parts of doing that. Having a way to specify (in a puppet spec test?) to run shell/Python checks on a template given certain inputs might be more achievable.~~ ~~We could also make this a wrapper around PCC for manual testing of a few key scripts, to start with, if that was easier. Here's a v0 proof of concept that, given PCC output to expand the templates, yoinks the script out of the post-change catalog and runs it through shellcheck (which identifies an issue that caused problems when the patch was merged):~~ ```% curl -s https://puppet-compiler.wmflabs.org/compiler1003/22927/puppetmaster1002.eqiad.wmnet/change.puppetmaster1002.eqiad.wmnet.pson \ | jq -r '.resources[] | select(.title == "/usr/local/bin/puppet-merge") | .parameters.content' \ | shellcheck - In - line 181: if [ $LABS_PRIVATE -eq 1 -a ${LABS_EXIT} -ne 99]; then ^-- SC1009: The mentioned syntax error was in this if expression. ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks. ^-- SC1020: You need a space before the ]. ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again. ``` === from T277892 > During Python 3 porting, I discovered a few Python 2 scripts that are erb files which get their configurations filled in through puppet. This makes them difficult to test and > maintain, so we should consider not doing that if possible. > > The scripts are these: > > modules/beta/templates/wmf-beta-autoupdate.py.erb > modules/profile/templates/hadoop/net-topology.py.erb > modules/profile/templates/hive/client/beeline_wrapper.py.erb

We have many scripts that are defined as ERB templates. We often make simple errors when editing them, as there's no tooling for dealing with such a format -- and in fact it's quite hard to write tooling for such, given the format of Puppet catalogs (which elides which template file produced a given generated file). Instead, let's forbid scripts being generated from templates. (And also set up `shellcheck` for .sh files.) ===== original/deprecated follows ===== ~~Ideally CI would automatically expand the templates as best it could*, and then run all the 'usual' checks we'd run for such a language. However, there's several tricky parts of doing that. Having a way to specify (in a puppet spec test?) to run shell/Python checks on a template given certain inputs might be more achievable.~~ ~~We could also make this a wrapper around PCC for manual testing of a few key scripts, to start with, if that was easier. Here's a v0 proof of concept that, given PCC output to expand the templates, yoinks the script out of the post-change catalog and runs it through shellcheck (which identifies an issue that caused problems when the patch was merged):~~ ```% curl -s https://puppet-compiler.wmflabs.org/compiler1003/22927/puppetmaster1002.eqiad.wmnet/change.puppetmaster1002.eqiad.wmnet.pson \ | jq -r '.resources[] | select(.title == "/usr/local/bin/puppet-merge") | .parameters.content' \ | shellcheck - In - line 181: if [ $LABS_PRIVATE -eq 1 -a ${LABS_EXIT} -ne 99]; then ^-- SC1009: The mentioned syntax error was in this if expression. ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks. ^-- SC1020: You need a space before the ]. ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again. ``` === from T277892 > During Python 3 porting, I discovered a few Python 2 scripts that are erb files which get their configurations filled in through puppet. This makes them difficult to test and > maintain, so we should consider not doing that if possible. > > The scripts are these: > > modules/beta/templates/wmf-beta-autoupdate.py.erb > modules/profile/templates/hadoop/net-topology.py.erb > modules/profile/templates/hive/client/beeline_wrapper.py.erb