Skip to content

Conversation

@jshigetomi
Copy link
Contributor

PR Summary

This pull request introduces support for the PowerShell "PSContentPath" experimental feature, allowing users to customize the location of user content (such as modules and scripts) via an environment variable or configuration file. The code now prefers these new paths when the feature is enabled, improving flexibility and future compatibility.

Support for PSContentPath experimental feature:

  • Added logic to detect if the "PSContentPath" experimental feature is enabled by checking the PowerShell configuration file (powershell.config.json).
  • When enabled, the code looks for a custom PSUserContentPath in the environment variable or configuration file, using it as the base path for user content if it exists. If not set, it defaults to LocalApplicationData\PowerShell.
  • If the experimental feature is not enabled, the code continues to use the legacy MyDocuments\PowerShell folder for user content.

Refactoring and path resolution:

  • Updated GetPathsFromEnvVarAndScope to use the new psUserContentPath variable instead of the legacy documents path, ensuring the correct directory is used based on feature detection.

Utility methods:

  • Introduced two new helper methods: IsExperimentalFeatureEnabled (to check for enabled features in the config) and GetPSUserContentPath (to retrieve the custom user content path from environment or config).

PR Context

PR Checklist

@jshigetomi
Copy link
Contributor Author

Added support for Linux and MacOS pathway as well

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alerickson
Copy link
Member

Opened PR to run CI tests here: #1913

@jshigetomi
Copy link
Contributor Author

Decided to use pwsh runspaces instead of reflection to call the PSContentPath API.
Talked to @TravisEz13 and found out that APIScan will require proof that APIs are public and not internal. So utilizing the new cmdlet Get-PSContentPath in a pwsh runspace to get the path from the config or default LOCALAppData location.

@alerickson
Copy link
Member

Tests are looking good, currently 3 failing tests related to Lucene index issues (ie these are flakey tests due to a server side issue with the Gallery). In terms of this PR all tests are passing!

@alerickson
Copy link
Member

All tests are passing now, PR looks good to me, just have one comment regarding whether to use default runspace created by the .Create() method or use current runspace.

if (psVersionObj != null) psVersion = new Version((int)psVersionObj.Major, (int)psVersionObj.Minor);
}
catch {
// Fallback if dynamic access fails
Copy link
Member

Choose a reason for hiding this comment

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

do we need to write a verbose message or warning?

}

allUsersDir = System.IO.Path.Combine("/usr", "local", "share", "powershell");
allUsersDir = Path.Combine("/usr", "local", "share", "powershell");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allUsersDir = Path.Combine("/usr", "local", "share", "powershell");
allUsersDir = Path.Combine("/", "usr", "local", "share", "powershell");

@alerickson
Copy link
Member

@jshigetomi we reviewed your PR, @anamnavi has a couple comments/suggestions. Everything looks good otherwise. Because we can't test these changes until 7.7 is released we're going to hold off on merging until then.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants