Bug #5081

Syntax checking not working with --ignoreimport

Added by Dan Carley over 2 years ago. Updated over 2 years ago.

Status:ClosedStart date:10/22/2010
Priority:HighDue date:
Assignee:-% Done:

0%

Category:executables
Target version:2.6.5
Affected Puppet version:2.6.1 Branch:
Keywords:iteration_2010-12-01

Description

The use of --ignoreimport with --parseonly isn’t raising errors on bad syntax. I’ve traced the last time this was working back to the tag of 2.6.0rc4. This is important for those of us who use syntax checks in commit hooks, in order to prevent buggy manifests from going out.

It can be observed with the following:

class test {
    file { "/tmp/foo":
        content => "bar",
        some invalid text
    }
}

dan@dan-MacPro:~/projects/puppet$ git describe
2.6.0rc4
dan@dan-MacPro:~/projects/puppet$ RUBYLIB=./lib ./bin/puppet --parseonly --ignoreimport ../test.pp; echo $?
err: Could not parse for environment production: Syntax error at 'invalid'; expected '}' at /home/dan/projects/test.pp:4
1

dan@dan-MacPro:~/projects/puppet$ git describe
2.6.3rc1-105-gb1ef091
dan@dan-MacPro:~/projects/puppet$ RUBYLIB=./lib ./bin/puppet --parseonly --ignoreimport ../test.pp
dan@dan-MacPro:~/projects/puppet$ echo $?
0

Related issues

Related to Puppet - Bug #4349: puppetdoc cannot parse site.pp Closed 07/23/2010
Related to Puppet - Bug #6634: puppet doc doesn't document resources in manifests other ... Accepted 03/08/2011

History

#1 Updated by Markus Roberts over 2 years ago

  • Status changed from Unreviewed to Needs More Information

This came in with commit:760e418d254a8d2198d2c6eb466d783a5930ef47 as the fix for #4349 and is arguably correct behaviour; why are you using ignoreimport here?

#2 Updated by Dan Carley over 2 years ago

Markus Roberts wrote:

why are you using ignoreimport here?

Good question. For which “it’s always worked that way” isn’t a very good answer :)

The intention is to ignore anything beyond the scope of a single manifest when checking the syntax of committed code. Because you won’t have the rest of the modulepath or even the current module available to you when looking within a single commit. This matches the description from the manpage:

A parameter that can be used in commit hooks, since it enables you to parse-check a single file rather than requiring that all files exist.

Having looked at it more carefully, it seems that:

  • --ignoreimport isn’t required when relying on auto-imports from include directives. Since they appear to be ignored when using --parseonly.
  • --ignoreimport is required if you have any explicit import directives in your manifests (of which I have some). Otherwise they will result No file(s) found for import of.

In terms of “least surprise”, it seems to me that --parseonly --ignoreimport should consider all classes within the manifest that it was told to operate on, but not anything that would be pulled in additionally with include or import.

#3 Updated by James Turnbull over 2 years ago

  • Category set to executables
  • Status changed from Needs More Information to Accepted
  • Assignee set to Nigel Kersten
  • Priority changed from Normal to High
  • Target version set to 69

So Dan is right … and the problem is worse than just —ignoreimport. —parseonly itself appears broken since 2.6.0.

#4 Updated by James Turnbull over 2 years ago

It was introduced in commit:a0ea74b5e8882e3f4e5cb1a1c396581e5484000e as far as I can see.

#5 Updated by James Turnbull over 2 years ago

  • Target version changed from 69 to 2.6.5

#6 Updated by Nigel Kersten over 2 years ago

  • Keywords set to iteration_2010-12-01

#7 Updated by Matt Robinson over 2 years ago

  • Status changed from Accepted to Merged - Pending Release
  • Assignee deleted (Nigel Kersten)

commit:616986da3751012cf526ad75fd250abc93e6c52a

#8 Updated by Nick Lewis over 2 years ago

  • Status changed from Merged - Pending Release to Closed

Also available in: Atom PDF