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.

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

(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

Conversation

justinstoller
Copy link
Member

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.

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.
@justinstoller justinstoller requested a review from a team as a code owner October 23, 2023 22:59
@justinstoller justinstoller self-assigned this Oct 23, 2023
@joshcooper joshcooper added the bug Something isn't working label Nov 15, 2023
@joshcooper
Copy link
Contributor

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

@justinstoller
Copy link
Member Author

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.

@@ -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

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

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

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

: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?

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:

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?

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.

2 participants