Arrow within case

[jfroche]

Hello,

java::oracle (puppetlabs-java) is failing during parsing. I guess due to case within the case.

Here is the error:

ParseError {errorPos = SourcePos {sourceName = "./modules/java/manifests/oracle.pp", sourceLine = Pos 197, sourceColumn = Pos 12} :| [], errorUnexpected = fromList [Tokens ('$' :| "")], errorExpected = fromList [Tokens (':' :| ":"),Tokens ('{' :| "")], errorCustom = fromList []} at # SourcePos {sourceName = "./modules/application/manifests/profile/jenkins/slave.pp", sourceLine = Pos 39, sourceColumn = Pos 5}

Here is the code:

 case $ensure {
    'present' : {
      archive { $destination :
        ensure       => present,
        source       => "${oracle_url}${release_major}-${release_minor}/${package_name}",
        cookie       => 'gpw_e24=http%3A%2F%2Fwww.oracle.com%2F; oraclelicense=accept-securebackup-cookie',
        extract_path => '/tmp',
        cleanup      => false,
        creates      => $creates_path,
        proxy_server => $proxy_server,
        proxy_type   => $proxy_type,
      }->
      case $::kernel {
        'Linux' : {
          exec { "Install Oracle java_se ${java_se} ${version}" :
            path    => '/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin',
            command => $install_command,
            creates => $creates_path,
          }
        }
        default : {
          fail ("unsupported platform ${::kernel}")
        }
      }
    }
    default : {
      notice ("Action ${ensure} not supported.")
    }
  }

code is here: https://github.com/puppetlabs/puppetlabs-java/blob/master/manifests/oracle.pp#L218

Do you think this is a quick fix ? Thank you !

[jfroche]

Uhm it might be the arrow in fact. I dropped the arrow and it works. Do you know the reason why we can't use an arrow (->) within a case ?

[bartavelle]

It is the arrow, and it is not a quick fix :(

[bartavelle]

I took a longer look. It would involve a large change, which I don't really have time for. I can give pointers to brave coders though!

[PierreR]

I have been trying to narrow the problem.

Correct me if I am wrong but chaining -> with a case statement will always fail to parse:

  file {
    '/etc/test':
      ensure => directory,
  } ->
  case $::kernel {
    'Linux' : {
      notify {'hello':}
    }
    default : {
      fail ("unsupported platform ${::kernel}")
    }
  }

As a first step, I will add a test case shortly.

[bartavelle]

The problem is here : https://github.com/bartavelle/language-puppet/blob/29bfb444cffcbda25accb1a4819e7bdcf5fdd7d6/Puppet/Parser.hs#L554-L615

The parsing code with "chains" (arrows) only handles simple resource declarations, resource collectors or resource references. It handles the logic of separating resource declaration from resource dependency. In order to fix it, a new statement type should be created, describing the chaining. However not that resource references are not statements, so it will require a special case anyway.

It is actually hard to solve because of the structure of the interpreter. Each statement-evaluating function returns the list of resources that have been declared in them. However, they do not return stuff like if there was a resource reference or a resource collectors. Resource references, not being statements, have no business being handled in the first place. Resource collectors are simply registered and are handled at a later stage.

[PierreR]

a new statement type should be created, describing the chaining

which chaining exactly ?

I believe "missing" the chains that involves resource declaration and conditional is ... bearable. Using arrow in such cases seems dubious to me in general.

Still even so it is quite annoying because even 'ignored modules' are parsed. So we force the user to fork any puppet modules that use such a syntax.

Two questions:

1. Does it make sense and is it easy to prevent 'ignored modules' from being parsed ?
2. Do you have a concrete example in mind of a 'arrow' chaining that we really should support ?

[bartavelle]

I don't know what you mean by ignored modules.

As for arrow chaining, it is useful to catching circular dependencies, and if you want to replace a puppet server with language-puppet.

[PierreR]

I don't know what you mean by ignored modules.

I mean the --ignoredmodules option of the puppetresources command line (or in the default.yaml file)

[bartavelle]

The proper fix would be to rewrite the parser and interpreter from the ground up. language-puppet is based on the previous model where there was a distinction between statements and values, and that kind of small problems will be present as long as it is the case.

Dropping chain parsing will not solve the problem, as this code will make no sense:

foo { ..} -> case $foo { default: { File['/a/b/c'] } }

Of course, if you push a change that drops chain parsing, I won't object, as I suppose you guys are the only users of the library.

[PierreR]

Thanks for the answer.

I can see that the line

foo { ..} -> case $foo { default: { File['/a/b/c'] } }

should indeed work but on the other hand I would personally avoid chaining with conditional.

Anyhow, as a first step I was actually thinking about dropping the parsing of ignored modules. This looks like a quick win for us and would make the issue less urgent on our side.

I don't really see the point of parsing modules that we won't evaluate but I might be missing something.

I currently won't have the time to rewrite the parser. Also I am not sure I feel confident enough to undertake such a complete rewriting of the parser myself ...

[bartavelle]

It might be useful to parse the ignored modules to know which classes or defines are valid, but what you propose seems like a good idea. I'll look into it.

[PierreR]

To give you a bit of context, we are running puppetresources as part of our continuous integration server.

When there is a parse error in an external puppet module, our users are completely blocked. Our sole leverage is to quickly fork the external module which is less than ideal.

We are only using --ignoredmodules as a last resort and usually the list contains none or few modules.

[bartavelle]

I just pushed an (untested) change. It should not parse ignored modules now. The reason it parsed them was that it could check that the parameters were correct.

[PierreR]

Seems to work perfectly so far.

Would you mind pushing the change on hackage ?

[bartavelle]

Done.

[PierreR]

Thanks. I think we should create a new issue about rewriting the parser so we don't forget about that. What do you think ?

[bartavelle]

Yup!