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

[justinstoller]

The previous implementation would not set the resolved_path variable of an environment to the actual resolved path unless the "report_configured_environment" was set to false. This should not matter since the externalize_path method will check the
"report_configured_environment" setting.

The implementation should probably either always set the resolved_path to the resolved path, or only set it if "versioned_environment_dirs" is true. This patch sets it only if the "versioned_environment_dirs" setting is true.

---

^ That is the commit message.

PR-wise, I came across this block while looking for something else and it looked wrong. It might be correct but I didn't find any meaningful reasoning that the jerk implementor left behind. I think we set Environment#resolved_path to some value regardless lest nils creep into the system, but in retrospect giving it an obviously wrong value doesn't seem any better. It also may be an artifact of a development iteration where Environment#externalize_path didn't check if resolved_path was set before acting on it.

I'll need to run some manual acceptance tests in PE to validate the change, but I will have to do that later.

[joshcooper]

Thanks @justinstoller, just checking if this is still needed?

[justinstoller]

Yeah, I think this is a good refactor for maintainability.

I've just run some versioned deploys with this change and re-reviewed the unit tests and don't think it should cause a regression.

[joshcooper]

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?