Skip to content

Conversation

@donoghuc
Copy link
Contributor

@donoghuc donoghuc commented May 7, 2020

This commit updates the catalog compiler to use updates to PAL api to enable node definitions in plans and deserialized bolt types in apply blocks without relying on using public methods. This includes a refactor to clean up the compile_catalog method and make it more readable.

nicklewis added a commit to nicklewis/bolt that referenced this pull request May 7, 2020
`bolt plan show <plan>` was failing when passed a project-level
plan due to the fact that the project path is a Pathname rather than a
string, unlike most modules. We now convert the path to a string in
`get_plan_info` so that any code interacting with the plan data can
treat it as if it came from any other module.

!bug

  * **Fix `bolt plan show <plan>` for project-level plans**
    ([puppetlabs#1799](puppetlabs#1798))

  This command was throwing errors due to a type mismatch that is now
  resolved.
@nicklewis
Copy link
Contributor

This makes sense to me.

@puppetcla
Copy link

CLA signed by all contributors.

@joshcooper joshcooper changed the title (PUP-1046) Use updated PAL api for catalog compilation (PUP-10476) Use updated PAL api for catalog compilation May 14, 2020
This commit updates the catalog compiler to use updates to PAL api to enable node definitions in plans and deserialized bolt types in apply blocks without relying on using public methods. This includes a refactor to clean up the compile_catalog method and make it more readable.

!no-release-note
@donoghuc donoghuc marked this pull request as ready for review June 3, 2020 16:44
@donoghuc donoghuc requested a review from a team as a code owner June 3, 2020 16:44
@donoghuc
Copy link
Contributor Author

donoghuc commented Jun 3, 2020

Depends on puppetlabs/puppet-runtime#343

@donoghuc donoghuc requested a review from nicklewis June 9, 2020 23:59
@donoghuc
Copy link
Contributor Author

donoghuc commented Jun 9, 2020

Puppet runtime is now shipping puppet 6.16.0

node_name_value: target['name'],
# CODEREVIEW: Isnt this already the default?
# https://github.com/puppetlabs/puppet/blob/master/lib/puppet/parser/catalog_compiler.rb#L18
rich_data: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and Puppet's default for this is true as well, and we shouldn't be picking up Puppet config anywhere. I think it's safe to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome. Removed.

bolt_project: bolt_project
}

# Facts will be set by the catalog compiler, so we need to ensure
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this actually belongs above the definition of shadow_vars, since this describes exactly what that function is doing.

Copy link
Contributor

@lucywyman lucywyman Jun 11, 2020

Choose a reason for hiding this comment

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

Or to above plan_vars =, whichever you think makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or removed altogether, the current comment above the function definition does a pretty good job of getting the important point across

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe just the target = request['target'] makes that a bit awkward. We need that target var there to access facts. The description i think is meant to descrive how and why topscope_vars is computed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought it might be more clear placed elsewhere, but I don't think it's worth holding this up for.

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.

4 participants