1375483 - Update wiki documentation and add commands for checking puppet code

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Comment 1

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_puppetcodechecktool.patch

A python script who parse .travis.yaml file and execute the puppet lint and puppet parser validate checks

Comment 2

[Jake Watkins [:dividehex]]

Comment on attachment 8882368 [details] [diff] [review]
Bug_1375483_checking_puppet_code_puppetcodechecktool.patch

Review of attachment 8882368 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 3

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_travis.patch

Changed lint checks skips and eliminated from the lint check puppetlabs modules

Comment 4

[Dragos Crisan [:dragrom]]

Comment on attachment 8882368 [details] [diff] [review]
Bug_1375483_checking_puppet_code_puppetcodechecktool.patch

https://hg.mozilla.org/build/puppet/rev/5b0f1b11e9d4742ad7a04199457a445c00f49f5b

Comment 5

[Jake Watkins [:dividehex]]

Comment on attachment 8883016 [details] [diff] [review]
Bug_1375483_checking_puppet_code_travis.patch

Review of attachment 8883016 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of formatting.  Please fix and resubmit.

Before making any changes to the travis config, an email will need to be crafted and sent out detailing the additional checks that are going to be put in place.  The style guide on the puppetagain wiki page (https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Hack_on_PuppetAgain) will need to also be updated.  And instructions should be given on how to run the same lint checks on a persons local host.

Let's make this a little more easier to read by putting each argument on a separate line
eg.

 - "puppet-lint \
     --no-140chars-check \
     --no-case_without_default-check \
...

Each module exclusion line should also be per line for readability.  A comment about the reason we are excluding certain modules might be helpful also.
eg.

     --log-format '%{path} - %{KIND}: %{message} on line %{line} - %{check}' \
     `find modules -name '*.pp' \
     -not -path 'modules/acl/*' \
     -not -path 'modules/assert/*' \
...

This also makes it easier to determine what is being added and/or removed in the future.

Comment 6

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_travis.patch

For the moment I put this patch only for feedback. After I'll finish the wiki documentation, I'll prepare the email and add this patch for review. Until then,I'll want to receive the feedback from you.

What I changed in .travis.yaml file:

 - added file from manifests files to check
 - finalized the skip checks from puppet lint
 - Arranged skipped checks and skipped modules for a better visibility and understand
 - Added comments about changes

Comment 7

[Jake Watkins [:dividehex]]

Comment on attachment 8883568 [details] [diff] [review]
Bug_1375483_checking_puppet_code_travis.patch

Review of attachment 8883568 [details] [diff] [review]:
-----------------------------------------------------------------

Looks much better! And thank you for the descriptive comments both inline and in-bug.  Good documentation and comments adds great value to your code. :-)
Just fix misspelling and tws.

::: .travis.yml
@@ +4,5 @@
>  
>  script:
>    #- "ln -s manifests/moco-config.pp manifests/config.pp"
>    #- "ln -s manifests/moco-nodes.pp manifests/nodes.pp"
> +  # Added manifests directory to validate. I skiped site.pp from check, to prevent errors related missing links 

Skipped is misspelled.  Also, trailing whitespace.

Comment 8

[Dragos Crisan [:dragrom]]

https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Hack_on_PuppetAgain

Comment 9

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_travis.patch

Finished the coding guideline documentation: https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/HowTo/Hack_on_PuppetAgain

What I changed in .travis.yaml file:

 - added file from manifests files to check
 - finalized the skip checks from puppet lint
 - Arranged skipped checks and skipped modules for a better visibility and understand
 - Added comments about changes

Comment 10

[Amy Rich [:arr] [:arich]]

The setup/test_puppetcode.py tries to import python libs that are not part of the base distribution. We should include a script to install those via pip and make sure that we specify any requirements (version, libs, etc) for python.

Comment 11

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_test_puppetcode.patch

In this version I made the following changes:

* Added check to see if pyyaml python module is installed or not. If is not installed, the script will install it.
* Installing puppet-lint and puppet 3.7.0 gems. The gem puppet-3.7.0 has limited compatibility with ruby version <=2.2. If this part will create more difficulties for the users, we can erase it
* Added documentation into the code.

Comment 12

[Jake Watkins [:dividehex]]

Comment on attachment 8884830 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

Review of attachment 8884830 [details] [diff] [review]:
-----------------------------------------------------------------

It isn't a very good idea to take on the burden of installing required pip or gem packages within the script itself.  You can assume the person who is executing this will also know how to install them themselves.  I would be pretty annoyed if I executed this and then preceded to watch it install global packages and make permanent changes to my systems.  Especially if it were calling sudo!  I (like many others) prefer to keep my environment fairly clean by using virtualenv, vagrant, docker, etc...  Instead of having the script make invasive changes, I would simply print a warning that the required dependent packages are missing and maybe short instructions/hints on how to install them.

Comment 13

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_test_puppetcode.patch

With this patch I changed the followings:
* Printed information about how to use this module and what tools need to be installed
* check if pyyaml module is installed. If is not installed the installation instructions will be displayed

Comment 14

[Jake Watkins [:dividehex]]

Comment on attachment 8885223 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

Review of attachment 8885223 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with small spelling fixes

::: setup/test_puppetcode.py
@@ +8,4 @@
>  import subprocess
> +import sys
> +
> +print 'This module parse the travis configuration file and execute the commands',\

Small grammar correction:  'parse' and 'execute' should be plural
eg. 'parses' and 'executes'

@@ +43,5 @@
> +                result = subprocess.check_output(script, shell=True, stderr=subprocess.STDOUT)
> +                # If script return with no errors, display the result
> +                print result
> +            except subprocess.CalledProcessError as e:
> +                # If the script return the error, this error will be printed on the console. By defolt, python

Typo in the word 'default'

Comment 15

[Jake Watkins [:dividehex]]

Comment on attachment 8884309 [details] [diff] [review]
Bug_1375483_checking_puppet_code_travis.patch

Review of attachment 8884309 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but make sure email is sent to relops and releng notifying everyone about the new lint requirements and this is landed on whatever date is decided upon and communicated in the email.
Looks good otherwise, just fix the tws

::: .travis.yml
@@ +4,5 @@
>  
>  script:
>    #- "ln -s manifests/moco-config.pp manifests/config.pp"
>    #- "ln -s manifests/moco-nodes.pp manifests/nodes.pp"
> +  # Added manifests directory to validate. I skiped site.pp from check, to prevent errors related missing links 

trailing whitespace

Comment 16

[Dragos Crisan [:dragrom]]

Comment on attachment 8885223 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

https://hg.mozilla.org/build/puppet/rev/230166fe2b45585f7866c3719a94cb59e37a931c

Comment 17

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_test_puppetcode.patch

With this patch I will do the followings:

* Add checks for puppet and puppet-lint presence
* Add checks for puppet version and puppet-lint version
* Create function for the checks and to print messages

Comment 18

[Amy Rich [:arr] [:arich]]

Here's what I'd like to see:

The script checks to make sure all of the necessary software is installed and at an adequate version (new enough, not too new, whatever). From what I saw, this includes ruby, python, puppet, puppet-lint, and any python libs you need.

If all of the correct software is installed, the only output of the script should be the output of the puppet-lint command. 

Add a --verbose/-v flag to optionally print out the puppet-lint command it's running.

If any software is missing or not the correct version, print out a list of the missing or incorrect version software and what the requirements are, then print a list of commands that need to be run to install the software which prompts the user to do the actual install. Think of the kind of output yum gives.

e.g.
=================================================================
puppet 3.7.0 required, python 2.7.5 installed
puppet-lint 2.2.1 required, puppet-lint not installed

Installing:
  sudo gem install puppet-lint -v 2.2.1
Updating:
  sudo gem install puppet 3.7.0

(If you wish to install these using your system package manager or another method, choose 'n' below)

Proceed with software installation/updates? [y/n]:
=================================================================

Note there's nothing in there about ruby gems or python libraries because they passed the version test.

If the user answers yes or y (or any capitalized variants thereof), install the software for them using the commands you printed out above. If the user answers no or n (or any capitalized variants thereof), bail out of the script.

There should be a --help flag that prints out what the script does and what arguments it takes (help and verbose, and any others).

Comment 19

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_checking_puppet_code_test_puppetcode.patch

With this patch I add the following:

* User interaction
* Possibility to automatically install/update the tools
* Split the code in procedures and functions and documented the code, for a better understanding

Comment 20

[Amy Rich [:arr] [:arich]]

Comment on attachment 8886554 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

I'm not sure this is worth a rewrite since to make it more elegant since it's a fairly simple script and we want to ship this, but I'll give feedback about the code structure.


I'd define the software version numbers you need at the top, that way they aren't hardcoded in throughout the script and can be easily changed if requirements change.

I'd change return_tool_version to be a comparison of the desired version and the existing version which then returns okay, upgrade, or install.

Then when you get to the section where you're asking people to conform to the requirements, you simply do a loop with 3 cases. If okay, do nothing, if upgrade, print out the upgrade commands, if install, print out the install commands (using a tuple of passed program name, version, and install method).

I'd use strtobool for the user prompt: https://docs.python.org/2/distutils/apiref.html?highlight=distutils.util#distutils.util.strtobool

Comment 21

[:dhouse]

Attachment: bug_1375483_create-puppet-lint.rc.patch

This moves the puppet-lint configuration out to an rc file so we don't have to parse/run the travis.yml and can run puppet-lint manually. As a developer, I would like to run `puppet-lint modules` in my workflow instead of the script. (Dragos, I tried to add you for review/feedback but bugzilla told me I cannot)

Comment 22

[:dhouse]

Comment on attachment 8886554 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

The code looks good, with clear comments and it makes sense what it does. However, I don't think we need to do this; I'm okay with keeping the script, but I'd rather tell the developers to run lint and set something to make it more painful if they do not.

Comment 23

[Jake Watkins [:dividehex]]

Comment on attachment 8886583 [details] [diff] [review]
bug_1375483_create-puppet-lint.rc.patch

Review of attachment 8886583 [details] [diff] [review]:
-----------------------------------------------------------------

I see where you are coming from here although, I don't think we need to split this into two files.  Having it in one file gives you the benefit of seeing the entire lint instructions in one spot.  You also run into the problem of precedent where someone may change the cmd in .travis.yml which ends up overriding the .rc or they may have their own .rc config being read in their env.   I think if an individual wants to deviate from what is defined in travis, they can write there own .rc and keep it local.

Good thinking though!  I didn't even consider that use of a config file for puppet-lint

Comment 24

[Jake Watkins [:dividehex]]

Comment on attachment 8886554 [details] [diff] [review]
Bug_1375483_checking_puppet_code_test_puppetcode.patch

Review of attachment 8886554 [details] [diff] [review]:
-----------------------------------------------------------------

I think this script has exploded in scope of work.  It went from a small script of simply parsing the .travis.yml and executing the commands in the 'script' array to this.  :dragrom, if you want feedback on the code itself, let's schedule a vidyo meeting when you get back from pto and I can give a much further in depth assessment of it.  I think that would be more useful than marking it up here.

For now, let's keep the script as it is already in the repo.  The environment problem can be solved with other tools such as a docker image.

Comment 25

[Dragos Crisan [:dragrom]]

Code is lint friendly and the wiki documentation was updated

Comment 26

[Dragos Crisan [:dragrom]]

:fubar Please let me know when you sent the email. After that I'll land Bug_1375483_checking_puppet_code_travis.patch

Comment 27

[Kendall Libby [:fubar] (he/him)]

Jake, is there a repo for your docker image? There's a discrepancy between what it's currently running for lint checks and what Dragos is proposing to use in Travis, e.g.:

$ diff travis docker
1a2
> --no-arrow_alignment-check \
2a4,6
> --no-documentation-check \
> --no-double_quoted_strings-check \
> --no-variable_scope-check \
3a8
> --no-variables_not_enclosed-check \
4a10
> --no-ensure_first_param-check \
5a12,16
> --no-variable_contains_dash-check \
> --no-parameter_order-check \
> --no-variable_is_lowercase-check \
> --no-arrow_on_right_operand_line-check \
> --no-file_mode-check \
10,14c21,23
< --no-file_mode-check \
< --no-variable_scope-check \
< --no-double_quoted_strings-check \
< --no-autoloader_layout-check \
< --log-format '%{path} - %{KIND}: %{message} on line %{line} - %{check}'
---
> --no-unquoted_resource_title \
> --no-names_containing_dash \
> --log-format '%{path} - %{KIND}: %{message} on line %{line} - %{check}' modules

Comment 28

[Kendall Libby [:fubar] (he/him)]

Oh, hah, it's actually using the .travis.yml. nvm, nothing to see here.

Comment 29

[Kendall Libby [:fubar] (he/him)]

Testing on my laptop:

sekrit (4d1ae4d)$ docker images
REPOSITORY                     TAG                 IMAGE ID            CREATED             SIZE
mozillarelops/puppet-linter    latest              38b16013e465        15 hours ago        58.85 MB

sekrit (4d1ae4d)$ hg log -l 1
changeset:   5746:4d1ae4dadd0b
tag:         tip
user:        Andrei Obreja <[e-mail]>
date:        Fri Sep 01 16:55:30 2017 +0300
summary:     remove modules/packages/manifests/mozilla/python27.pp.orig

sekrit (4d1ae4d)$ docker run --rm -v ~/Work/repo/hg/build/puppet:/puppet mozillarelops/puppet-linter
Executing: puppet parser validate `find modules -name '*.pp'`
Error: Could not parse for environment production: Could not match  at /puppet/modules/fw/manifests/networks.pp:171
Executing: puppet-lint --no-arrow_alignment-check --no-140chars-check --no-documentation-check --no-double_quoted_strings-check --no-variable_scope-check --no-case_without_default-check --no-variables_not_enclosed-check --no-only_variable_string-check --no-ensure_first_param-check --no-unquoted_file_mode-check --no-variable_contains_dash-check --no-parameter_order-check --no-variable_is_lowercase-check --no-arrow_on_right_operand_line-check --no-file_mode-check --no-nested_classes_or_defines --no-quoted_booleans --no-puppet_url_without_modules --no-selector_inside_resource --no-unquoted_resource_title --no-names_containing_dash --log-format '%{path} - %{KIND}: %{message} on line %{line} - %{check}' modules
modules/fw/manifests/networks.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 171 - syntax
modules/fw/manifests/profiles/osx_taskcluster_worker.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 17 - syntax
modules/fw/manifests/profiles/partner_repack.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 17 - syntax
modules/fw/manifests/roles/syslog_from_scl3_releng.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 17 - syntax
modules/packages/manifests/mozilla/python27.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 120 - syntax
modules/releaserunner/manifests/init.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 109 - syntax
modules/taskcluster_host_secrets/manifests/init.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 45 - syntax
modules/toplevel/manifests/server/taskcluster_host_secrets.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 10 - syntax
modules/toplevel/manifests/slave/releng.pp - ERROR: Syntax error (try running `puppet parser validate <file>`) on line 56 - syntax

sekrit (4d1ae4d)$ puppet --version
3.7.0
sekrit (4d1ae4d)$ puppet parser validate modules/fw/manifests/networks.pp
sekrit (4d1ae4d)$ puppet parser validate modules/fw/manifests/profiles/osx_taskcluster_worker.pp
sekrit (4d1ae4d)$ puppet parser validate modules/fw/manifests/profiles/partner_repack.pp
...etc...

Looking at the files complaining about syntax errors, I don't see anything obviously incorrect, and `puppet parser validate` doesn't show any errors/warnings. As it stands, I don't think this is going to work well for general use.

How do we get a better error message to the user to show what needs to be fixed? Can the linter do that (can it be made to also run `puppet parser validate`?), or do we need to provide another method?

Comment 30

[Jake Watkins [:dividehex]]

I don't get any of those error when I run the container against my local repo.  Have you tried pulling a fresh clone to another dir and run it against that?  The puppet parser should have picked up those syntax errors before the linter.  What about running the previous image (dividehex/releng-puppet-test) against it?  Does that work?  Which version of docker are you using?

Did this change only start to fail when you applied the proposed .travis.yml changes?

Comment 31

[Kendall Libby [:fubar] (he/him)]

Bizarre. After checking everything, I updated docker-machine and it's working now. *docker* didn't change versions, but boot2docker did, plus it restarted docker-machine. I'm having difficulty with how that caused the above errors, but here we are.

Comment 32

[Dragos Crisan [:dragrom]]

docker run --rm -v ~/repositories/puppet:/puppet mozillarelops/puppet-linter - works for me without errors

Comment 33

[Dragos Crisan [:dragrom]]

Attachment: Bug_1375483_Little_clean_on_puppet_code.patch

Corrected soft_tabs error on manifests/moco_config.pp
Arranged 4 arrows on modules/bacula_client/manifests/init.pp files:
manifests/moco-config.pp - ERROR: two-space soft tabs not used on line 314 - 2sp_soft_tabs
modules/bacula_client/manifests/init.pp - WARNING: indentation of => is not properly aligned (expected in column 29, but found it in column 31) on line 21 - arrow_alignment
modules/bacula_client/manifests/init.pp - WARNING: indentation of => is not properly aligned (expected in column 29, but found it in column 31) on line 22 - arrow_alignment
modules/bacula_client/manifests/init.pp - WARNING: indentation of => is not properly aligned (expected in column 29, but found it in column 31) on line 23 - arrow_alignment
modules/bacula_client/manifests/init.pp - WARNING: indentation of => is not properly aligned (expected in column 29, but found it in column 31) on line 24 - arrow_alignment
modules/bacula_client/manifests/init.pp - WARNING: indentation of => is not properly aligned (expected in column 29, but found it in column 31) on line 25 - arrow_alignment

Comment 34

[Dragos Crisan [:dragrom]]

Comment on attachment 8905001 [details] [diff] [review]
Bug_1375483_Little_clean_on_puppet_code.patch

https://hg.mozilla.org/build/puppet/rev/498bf63fe558182dc95002d4b5fcd439b899915b

Comment 35

[Dragos Crisan [:dragrom]]

Comment on attachment 8884309 [details] [diff] [review]
Bug_1375483_checking_puppet_code_travis.patch

https://hg.mozilla.org/build/puppet/rev/9c27c69a4316e3cbc2791168990ee7ec948027bc