Skip to content

Fix regex node lookup regression from PUP-11515#481

Draft
corporate-gadfly wants to merge 2 commits into
OpenVoxProject:mainfrom
corporate-gadfly:complex-regex
Draft

Fix regex node lookup regression from PUP-11515#481
corporate-gadfly wants to merge 2 commits into
OpenVoxProject:mainfrom
corporate-gadfly:complex-regex

Conversation

@corporate-gadfly

Copy link
Copy Markdown
Contributor

Short description

The LOOKAROUND_OPERATORS tokens introduced in 5d09d7f are uppercase, but TypeCollection#munge_name() lowercases all lookup keys, so regex nodes with group syntax (e.g. (?:-test)) were stored under a key that could never be found again, producing "Cannot find definition Node".

Fix: downcase the generated synthetic name before storing it.

Fixes: #14

Checklist

I have:

@corporate-gadfly corporate-gadfly marked this pull request as draft June 9, 2026 23:26
@corporate-gadfly corporate-gadfly added the bug Something isn't working label Jun 10, 2026
@corporate-gadfly corporate-gadfly force-pushed the complex-regex branch 3 times, most recently from 903a80a to 7b6ffab Compare June 17, 2026 00:28
- The LOOKAROUND_OPERATORS tokens introduced in 5d09d7f are uppercase,
  but TypeCollection#munge_name() lowercases all lookup keys, so regex
  nodes with group syntax (e.g. (?:-test)) were stored under a key that
  could never be found again, producing "Cannot find definition Node".

  Fix: downcase the generated synthetic name before storing it.

  Fixes: OpenVoxProject#14

Signed-off-by: Corporate Gadfly <haroon.rafique@gmail.com>
Co-authored-by: GitHub Copilot <noreply@github.com>
Instead of replacing with uppercase keys and then downcasing the result, let's just use lowercase to begin with.

(I don't see how this would affect anything in the test results, but I don't know why the original test is failing either.)
@binford2k

Copy link
Copy Markdown
Contributor

This is a relatively harmless failure. It just means that on Windows / Ruby 3.2 that the generate face will do more work than it has to do. It should only be generating types for changed files, but with this change it's generating for all. It's still generating properly.

I don't have time to keep digging just now, but you might poke at

# Determines if the output file is up-to-date with respect to the input file.
# @param [String, nil] The path to output to, or nil if determined by input
# @return [Boolean] Returns true if the output is up-to-date or false if not.
def up_to_date?(outputdir)
f = effective_output_path(outputdir)
Puppet::FileSystem.exist?(f) && (Puppet::FileSystem.stat(@path) <=> Puppet::FileSystem.stat(f)) <= 0
end

Either something about this patch changes how the path is calculated or something about this patch changes how mtime works.

The only link I found between node name and the generate types command is that it uses Puppet::Node::Environment to identify where to look at files. If you have a Windows node handy, you might see if there's any difference in Puppet.lookup(:current_environment) in the context of that test.

@corporate-gadfly

Copy link
Copy Markdown
Contributor Author

Thankfully, my life is devoid of Windows at the moment 🤷.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Puppet 8 introduced a regex node definition regression

2 participants