Skip to content

Conversation

@mcdonaldseanp
Copy link
Contributor

This commit makes two changes to PAL to support more features within puppet during a catalog compile:

  1. When variables are passed to .with_catalog_compiler they are now deserialized if they are serialized pcore types
  2. A new function was added to the compiler class within .with_catalog_compiler so that a process calling PAL functions can evaluate ast with node definitions.

Previously, when using functions like .with_catalog_compiler and setting
variables the variables sent were assumed to be simple types. If the vars sent
are serialized PCore, we should attempt to deserialize before setting them.
@mcdonaldseanp mcdonaldseanp requested review from a team May 7, 2020 16:18
@donoghuc
Copy link
Contributor

donoghuc commented May 7, 2020

Raised this draft to show how we might use it in bolt. puppetlabs/bolt#1798

# In case the varaibles passed to the compiler are PCore types defined in modules, they
# need to be deserialized and added from within the this scope, so that loaders are
# available during deserizlization.
add_variables(compiler.topscope, Puppet::Pops::Serialization::FromDataConverter.convert(pal_variables))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cant we add give

class CatalogCompiler < Compiler
an add_variables method? Then we could just call that in bolt/puppetserver?

Copy link
Contributor Author

@mcdonaldseanp mcdonaldseanp May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but it seems incorrect to me that PAL would provide parameters for setting variables and not have them work for anything other than core puppet datatypes.

If PAL supports providing variables as a parameter to .with_catalog_compiler it should support those variables if they are datatypes defined in modules too.

This commit adds evaluate_node_ast as a public function available to the
catalog compiler inside PAL's .with_catalog_compiler.

When using .with_catalog_compiler, the calling process can now call
compiler.evaluate_ast_node to compile any node definitions inside the manifest
@justinstoller
Copy link
Member

I'm 💯 on updating PAL if it's not working for Bolt. Though I'll leave it to @puppetlabs/bolt to review if this is correct for them.

@donoghuc donoghuc requested a review from nicklewis May 7, 2020 18:41
@puppetcla
Copy link

CLA signed by all contributors.

@nicklewis
Copy link
Contributor

👍 This is great.

Copy link

@steveax steveax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mcdonaldseanp mcdonaldseanp merged commit 16e556f into puppetlabs:master May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants