Add external facts of top scope vars by alvagante · Pull Request #319 · example42/psick

alvagante · 2026-03-27T09:15:12Z

Before submitting your PR

Open an issue and refer to its number in your PR title
If it's a bug and you have the solution, go on with the PR!
If it's a new profile got with the PR (possibly with docs, samples and tests)

After submitting your PR

Verify Travis checks and fix the errors if needed
Feel free to ping us if we don't reply promptly

Copilot

Pull request overview

This PR updates the Puppet control-repo/module testing and classification setup to support generating “external facts” from top-scope variables (role/env/zone/datacenter/application), and adds Onceover + host compile specs targeting a lab factset.

Changes:

Update manifests/site.pp to derive top-scope vars from trusted facts/Hiera and emit external facts.
Rework spec/spec_helper.rb default facts loading/merging and tighten spec settings (e.g., strict variables, coverage report).
Add lab Onceover config + new host compile specs and a helper script to run Onceover in a container.

Reviewed changes

Copilot reviewed 7 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`spec/spec_helper.rb`	Refactors spec configuration and default fact loading/merging.
`spec/onceover.yaml`	Simplifies Onceover matrix to a single `lab` nodeset and enables `auto_vendored`.
`spec/hosts-lab/lab.psick.io/build.lab.psick.io_spec.rb`	Adds a host compile spec for the `build` role.
`spec/hosts-lab/lab.psick.io/jenkins.lab.psick.io_spec.rb`	Adds a host compile spec for the `jenkins` role (OS subset).
`spec/hosts-lab/lab.psick.io/puppet.lab.psick.io_spec.rb`	Adds a host compile spec for the `puppet` role (RedHat only).
`spec/hosts-lab/lab.psick.io/windows2012.lab.psick.io_spec.rb`	Adds a host compile spec named for Windows 2012.
`run_onceover.sh`	Adds a Podman-based wrapper to run Onceover.
`manifests/site.pp`	Adds top-scope var fallback logic + external-fact resource declarations.
`hiera_pabawi.yaml`	Introduces a Hiera config intended to use the new external facts in hierarchy paths.
`Puppetfile`	Comments out a number of optional modules (and adds clarifying comments).
`keys`	Removes a file containing a keys path reference.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T09:22:48Z

spec/spec_helper.rb

-rescue LoadError => loaderror
-  warn "Could not require spec_helper_local: #{loaderror.message}"
-end
+require 'spec_helper_local' if File.file?(File.join(File.dirname(__FILE__), 'spec_helper_local.rb'))


spec_helper_local is now required without a rescue. If spec_helper_local.rb exists but has missing dependencies or other load issues, the entire spec run will error (previous behavior was to warn and continue). Consider reinstating a rescue LoadError (or broader) around this optional require so local overrides don't break CI runs.

Suggested change

require 'spec_helper_local' if File.file?(File.join(File.dirname(__FILE__), 'spec_helper_local.rb'))

begin

require 'spec_helper_local' if File.file?(File.join(File.dirname(__FILE__), 'spec_helper_local.rb'))

rescue LoadError, StandardError => e

warn "WARNING: Unable to load spec_helper_local.rb: #{e}"

end

Copilot · 2026-03-27T09:22:49Z

manifests/site.pp

+  if ! getvar('facts.role') {
+    $role = pick_default(getvar('trusted.extensions.pp_role'), lookup('role', Optional[String], 'first', undef))
  }
-  if $trusted['extensions']['pp_environment'] and ! getvar('facts.env') {
-    $env = $trusted['extensions']['pp_environment']
+  if ! getvar('facts.env') {
+    $env = pick_default(getvar('trusted.extensions.pp_environment'), lookup('env', Optional[String], 'first', undef))
  }
-  if $trusted['extensions']['pp_datacenter'] and ! getvar('facts.datacenter') {
-    $datacenter = $trusted['extensions']['pp_datacenter']
+  if ! getvar('facts.datacenter') {
+    $datacenter =  pick_default(getvar('trusted.extensions.pp_datacenter'), lookup('datacenter', Optional[String], 'first', undef))
  }
-  if $trusted['extensions']['pp_zone'] and ! getvar('facts.zone') {
-    $zone = $trusted['extensions']['pp_zone']
+  if ! getvar('facts.zone') {
+    $zone = pick_default(getvar('trusted.extensions.pp_zone'), lookup('zone', Optional[String], 'first', undef))
  }
-  if $trusted['extensions']['pp_application'] and ! getvar('facts.application') {
-    $application = $trusted['extensions']['pp_application']
+  if ! getvar('facts.application') {
+    $application = pick_default(getvar('trusted.extensions.pp_application'), lookup('application', Optional[String], 'first', undef))


$role/$env/$datacenter/$zone/$application are only assigned inside the if ! getvar('facts.*') branches, but they are always referenced later when creating psick::puppet::set_external_fact resources. With strict_variables enabled (as in spec/spec_helper.rb), this will raise an undefined variable error whenever the corresponding fact is already set. Define these variables unconditionally (e.g., by starting from the existing fact value and then falling back to trusted/hiera) before using them.

Copilot · 2026-03-27T09:22:49Z

hiera_pabawi.yaml

+      - "role/%{fact_role}-%{fact_env}.yaml"
+      - "role/%{fact_role}.yaml"
+      - "zone/%{fact_zone}.yaml"


The hierarchy paths use %{fact_role}, %{fact_env}, and %{fact_zone} interpolation, but this repo's existing hiera.yaml uses explicit top-scope (%{::role}) and other dotted forms (%{trusted.certname}). To avoid relying on legacy fact-variable behavior (and to be consistent), prefer %{facts.fact_role}/%{facts.fact_env}/%{facts.fact_zone} (or %{::fact_role} etc.) so interpolation reliably resolves from facts.

Suggested change

- "role/%{fact_role}-%{fact_env}.yaml"

- "role/%{fact_role}.yaml"

- "zone/%{fact_zone}.yaml"

- "role/%{facts.fact_role}-%{facts.fact_env}.yaml"

- "role/%{facts.fact_role}.yaml"

- "zone/%{facts.fact_zone}.yaml"

Copilot · 2026-03-27T09:22:49Z

spec/spec_helper.rb

+default_fact_files.each do |f|
+  next unless File.exist?(f) && File.readable?(f) && File.size?(f)
+
+  begin
+    require 'deep_merge'
+    default_facts.deep_merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true))


The begin/rescue around the YAML merge will not catch LoadError from require 'deep_merge' (since LoadError is not a StandardError). If deep_merge isn't available in the bundle, specs will crash before emitting the warning. Either require deep_merge outside the block and handle LoadError, add it as an explicit dev dependency, or fall back to a non-deep merge when it's missing.

Suggested change

default_fact_files.each do |f|

next unless File.exist?(f) && File.readable?(f) && File.size?(f)

begin

require 'deep_merge'

default_facts.deep_merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true))

deep_merge_available = false

begin

require 'deep_merge'

deep_merge_available = true

rescue LoadError => e

RSpec.configuration.reporter.message "WARNING: deep_merge gem not available: #{e}; defaulting to non-deep merge of default facts."

end

default_fact_files.each do |f|

next unless File.exist?(f) && File.readable?(f) && File.size?(f)

begin

file_facts = YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true) || {}

if deep_merge_available

default_facts.deep_merge!(file_facts)

else

default_facts.merge!(file_facts)

end

Copilot · 2026-03-27T09:22:49Z

run_onceover.sh

@@ -0,0 +1,2 @@
+#!/bin/bash
+podman run --rm -v $(pwd):/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"


The podman run -v $(pwd):/repo mount isn't quoted, so it will break if the working directory path contains spaces. Quote $(pwd) (and consider set -euo pipefail) to make the script more robust.

Suggested change

podman run --rm -v $(pwd):/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"

set -euo pipefail

podman run --rm -v "$(pwd)":/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"

Copilot · 2026-03-27T09:22:50Z

spec/spec_helper.rb

 include RspecPuppetFacts

-# Custom support path
-support_path = File.expand_path(File.join(File.dirname(__FILE__), '..', 'spec/support/*.rb'))
-Dir[support_path].each {|f| require f}
-
 default_facts = {
  puppetversion: Puppet.version,
  facterversion: Facter.version,


Support files under spec/support are no longer being required. This breaks specs that rely on helpers/constants from those files (e.g., OS_FACTS used by the new spec/hosts-lab/** specs). Re-add the spec/support/*.rb requires (or explicitly require the needed support files) so the helper code is loaded during unit specs.

Copilot · 2026-03-27T09:22:50Z

manifests/site.pp

+  psick::puppet::set_external_fact { 'fact_role':
+    value => $role,
+  }
+  psick::puppet::set_external_fact { 'fact_env':
+    value => $env,
+  }
+  psick::puppet::set_external_fact { 'fact_datacenter':
+    value => $datacenter,
+  }
+  psick::puppet::set_external_fact { 'fact_zone':
+    value => $zone,
+  }
+  psick::puppet::set_external_fact { 'fact_application':
+    value => $application,
+  }


The manifest declares resources of type psick::puppet::set_external_fact, but there is no definition for this class/defined type in this repository (no psick::puppet::set_external_fact found under manifests/). As-is, catalog compilation will fail with an unknown resource type unless that type is added or the code is switched to an existing implementation.

Suggested change

psick::puppet::set_external_fact { 'fact_role':

value => $role,

}

psick::puppet::set_external_fact { 'fact_env':

value => $env,

}

psick::puppet::set_external_fact { 'fact_datacenter':

value => $datacenter,

}

psick::puppet::set_external_fact { 'fact_zone':

value => $zone,

}

psick::puppet::set_external_fact { 'fact_application':

value => $application,

}

# Note: the helper defined type `psick::puppet::set_external_fact` is not available in this repository,

# so the external fact creation resources have been disabled here to allow catalog compilation.

Add external facts of top scope vars

452b047

Copilot AI review requested due to automatic review settings March 27, 2026 09:15

Copilot started reviewing on behalf of alvagante March 27, 2026 09:15 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external facts of top scope vars#319

Add external facts of top scope vars#319
alvagante wants to merge 1 commit intomainfrom
onceover

alvagante commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-default_fact_files.each do |f|
-  next unless File.exist?(f) && File.readable?(f) && File.size?(f)
-  begin
-    require 'deep_merge'
-    default_facts.deep_merge!(YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true))
+deep_merge_available = false
+begin
+  require 'deep_merge'
+  deep_merge_available = true
+rescue LoadError => e
+  RSpec.configuration.reporter.message "WARNING: deep_merge gem not available: #{e}; defaulting to non-deep merge of default facts."
+end
+default_fact_files.each do |f|
+  next unless File.exist?(f) && File.readable?(f) && File.size?(f)
+  begin
+    file_facts = YAML.safe_load(File.read(f), permitted_classes: [], permitted_symbols: [], aliases: true) || {}
+    if deep_merge_available
+      default_facts.deep_merge!(file_facts)
+    else
+      default_facts.merge!(file_facts)
+    end

		@@ -0,0 +1,2 @@
		#!/bin/bash
		podman run --rm -v $(pwd):/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"

	podman run --rm -v $(pwd):/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"
	set -euo pipefail
	podman run --rm -v "$(pwd)":/repo --userns=keep-id puppet/puppet-dev-tools:4.x onceover "$@"

Conversation

alvagante commented Mar 27, 2026

Before submitting your PR

After submitting your PR

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants