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

feat: added export option to secrets env #342 #348

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from
+47 −1
Open

feat: added export option to secrets env #342 #348

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0f4664
feat: added export option to secrets env
Sooryasanand Dec 18, 2024
63a51c5
Merge branch 'MatrixAI:staging' into feature-env-export
Sooryasanand Jan 6, 2025
db99953
Added test for env export
Sooryasanand Jan 6, 2025
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
6 changes: 5 additions & 1 deletion src/secrets/CommandEnv.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,11 @@ class CommandEnv extends CommandPolykey {
// Formatting as a .env file
let data = '';
for (const [key, value] of Object.entries(envp)) {
data += `${key}='${value}'\n`;
if (options.envExport) {
data += `export ${key}='${value}'\n`;
} else {
data += `${key}='${value}'\n`;
}
Comment on lines +286 to +290
Copy link
Author

Choose a reason for hiding this comment

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

The if statement applies to Unix systems. On Windows, an equivalent check is unnecessary because the export keyword (or equivalent) isn’t needed. On Windows, child processes automatically inherit the parent process's environment variables, as documented on the official Microsoft website: Environment Variables.

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case, then instead of making this an option, we can probably make this another env format - something like --env-format unix-export. Or we can also rename this to be --env-format unix-local for non-export and --env-format unix-global for exported environment variables. Thoughts, @CMCDragonkai?

Copy link
Member

Choose a reason for hiding this comment

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

PowerShell does have an export like semantic. But I forgot the details.

I think we should automate the default expected behaviour as much as possible and minimise configuration options.

Copy link
Author

Choose a reason for hiding this comment

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

There are three ways to manage environment variables in PowerShell.

Solution 1: Setting Environment Variables at the Machine Level

You can set environment variables at the machine level using the following command:
[Environment]::SetEnvironmentVariable('${key}', '${value}', 'Machine')

However, changes made this way require a restart of the terminal session or new processes to take effect, which might not be ideal for immediate changes.

Solution 2: Using a Function to Add Paths to $env:Path

You can create a PowerShell function that:

  • Updates $env:Path in the current session so changes take effect immediately.
  • Persists the change for future sessions.
  • Avoids adding duplicate paths if they already exist.

Here’s an example function that implements similar function from stack overflow:

function Add-EnvPath {
    param(
        [Parameter(Mandatory=$true)]
        [string] $Path,

        [ValidateSet('Machine', 'User', 'Session')]
        [string] $Container = 'Session'
    )

    if ($Container -ne 'Session') {
        $containerMapping = @{
            Machine = [EnvironmentVariableTarget]::Machine
            User = [EnvironmentVariableTarget]::User
        }
        $containerType = $containerMapping[$Container]

        $persistedPaths = [Environment]::GetEnvironmentVariable('Path', $containerType) -split ';'
        if ($persistedPaths -notcontains $Path) {
            $persistedPaths = $persistedPaths + $Path | where { $_ }
            [Environment]::SetEnvironmentVariable('Path', $persistedPaths -join ';', $containerType)
        }
    }

    $envPaths = $env:Path -split ';'
    if ($envPaths -notcontains $Path) {
        $envPaths = $envPaths + $Path | where { $_ }
        $env:Path = $envPaths -join ';'
    }
}

This solution ensures that the environment variables are updated for both the current session and future sessions, without causing duplication.

Solution 3: Dynamic Update Based on envExport Flag

If you want to set the environment variables immediately in the current session and optionally persist them system-wide based on the envExport flag, you can use the following approach:

  • If envExport is true, the variable is set at the machine level and immediately available in the current session.
  • If envExport is false, the variable is only set for the current session.

Here’s the implementation:

case 'powershell': {
  let data = '';
  for (const [key, value] of Object.entries(envp)) {
    if (options.envExport) {
      // Export environment variable at the machine level (system-wide)
      data += `[Environment]::SetEnvironmentVariable('${key}', '${value}', 'Machine')\n`;
    } else {
      // Set environment variable for current session
      data += `$env:${key} = '${value}'\n`;
    }
  }

  // After setting the variables, fetch and update them in the current session
  for (const key of Object.keys(envp)) {
    data += `$env:${key} = [System.Environment]::GetEnvironmentVariable('${key}', 'Machine')\n`;
  }

  process.stdout.write(
    binUtils.outputFormatter({
      type: 'raw',
      data,
    }),
  );
}
break;

This solution allows flexibility: you can choose to export the variable system-wide or just for the current session, with immediate effect in the session.

Summary of Solutions:

  • Set machine-wide variables using [Environment]::SetEnvironmentVariable (requires restart of terminal or processes).
  • Use a PowerShell function to add paths to $env:Path, making changes immediately available in the current session and persistent for future sessions, while avoiding duplicates.
  • Use the envExport flag to control whether the variable is set system-wide (machine level) or just for the current session, updating the current session immediately.

}
process.stdout.write(
binUtils.outputFormatter({
Expand Down
6 changes: 6 additions & 0 deletions src/utils/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ const parents = new commander.Option(
'If enabled, create all parent directories as well. If the directories exist, do nothing.',
).default(false);

const envExport = new commander.Option(
'--export',
'If enabled, the export parameter sets the environment variable system-wide.',
).default(false);

const preserveNewline = new commander.Option(
'--preserve-newline <path>',
'Preserve the last trailing newline for the secret content',
Expand Down Expand Up @@ -372,5 +377,6 @@ export {
order,
recursive,
parents,
envExport,
preserveNewline,
};