Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(maint) Only resolve environment dirs if versioned dirs are enabled #9131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

(maint) Only resolve environment dirs if versioned dirs are enabled #9131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/puppet/environments.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,8 @@ def create_environment(name)

configured_path = File.join(@environment_dir, name.to_s)
env.configured_path = configured_path
if Puppet.settings[:report_configured_environmentpath]
if Puppet.settings[:versioned_environment_dirs]
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry coming back to this. In puppet8, we have

puppet/lib/puppet/defaults.rb

Lines 430 to 432 in 5503fe2

:report_configured_environmentpath => {
:type => :boolean,
:default => true,

So currently we always call validated_directory to resolve any symlinks in the environment directory

puppet/lib/puppet/environments.rb

Lines 255 to 259 in 7d8b294

def validated_directory(envdir)
env_name = Puppet::FileSystem.basename_string(envdir)
envdir = Puppet::Environments::Directories.real_path(envdir).to_s
if Puppet::FileSystem.directory?(envdir) && Puppet::Node::Environment.valid_name?(env_name)
envdir

However, we also have

puppet/lib/puppet/defaults.rb

Lines 264 to 265 in 5503fe2

:versioned_environment_dirs => {
:default => false,

So this PR would change the default behavior for resolved_path, which I think affects the file that the resource is associated with during reporting?

puppet/lib/puppet/resource.rb

Line 337 in 7d8b294

self.file = environment.externalize_path(attributes[:file])

A separate wrinkle is we have code that checks whether the configured_path and resolved_path are both set to avoid comparing the two strings:

puppet/lib/puppet/node/environment.rb

Lines 195 to 196 in 7d8b294

paths_set = configured_path && resolved_path
munging_possible = paths_set && configured_path != resolved_path

So a side effect of setting env.resolved_path = ..., is that paths_set will always be true. Not sure we want to do that or if we should short-circuit the externalize_path method in cases where versioned code deploys is disabled?

env.resolved_path = validated_directory(configured_path)
else
env.resolved_path = configured_path
end

env
Expand Down