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

Secret env command #265

Closed
wants to merge 4 commits into from
Closed

Secret env command #265

wants to merge 4 commits into from

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Oct 25, 2021

Description

This PR aims to refactor the existing pk secret env command according to the specifications outlined in the Development Environment Usecase. Essentially the command should be able inject environment variable or allow sourcing variables into an existing subshell.

Issues Fixed

Tasks

  1. Ensure that pk secrets env can be used to inject environment variables and run a subprocess
  2. Ensure that pk secrets env can be used to source environment variables and output something that can be used as a file to be sourced by a shell
  3. Handle export flag
  4. Handle globbing options specified in the usecase
  5. Handle command options with spaces (may extend to other secret and vault CLI commands)
  6. Extending globbing to match documentation here (https://git-scm.com/docs/gitignore)
  7. Testing for secret env command executing a subshell
  8. Add platform dependent subshell testing
  9. Deal with the fact that nodejs doesn't have an exec that can replace the current node process

Final checklist

  • Domain specific test (npm test -- tests/bin/secret.test.ts)
  • Full tests with npm test
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

Recreated this from: #210.

@scottmmorris
Copy link
Contributor

At the moment I am testing the basic exporting of secrets into the current shell by echoing the variable name:

nexpect.spawn('echo', ['$TEST_VAR_3']).expect('test-3');

However, exporting secrets into a subshell I've had to do a bit differently:

nexpect.spawn('npm', ['run', 'polykey', '--', 'secrets', 'env', '-np', dataDir, '-e', 'Vault000:**/*', 'bash'])
  .sendline('echo $TEST_VAR_2')
  .sendline('exit')
  .run(function (_, stdout) {
    expect(stdout).toContain('test-2');
    done();
  });

@CMCDragonkai
Copy link
Member Author

So nexpect does work on Windows. However bash won't be on Windows. So either we use a common shell, which there is none. Or you have to do a feature test see if bash exists, then use bash, and if not, try with powershell and then do the test.

@CMCDragonkai
Copy link
Member Author

The kexec is just an NPM library, you should be able to bring it in and just use it on Linux. On Windows we would continue to use child process as normal.

@scottmmorris
Copy link
Contributor

For kexec I forgot that you mentioned here that it has been abandoned and the latest release doesn't work on Node 12 and 14. But an unmerged branch has the required fixes. See here: https://github.com/MatrixAI/js-polykey/issues/205#issuecomment-876942149

So we were going to pull the source code into the polykey library in order to fix this instead of just npm installing it

@joshuakarp
Copy link
Contributor

When revisiting this, it will be worthwhile to take another look at the discussion had with Scott: https://vimeo.com/manage/videos/654376139

@CMCDragonkai
Copy link
Member Author

When addressing secrets related commands, we should review this too @tegefaulkes.

@CMCDragonkai
Copy link
Member Author

Superseded by #505.

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

Successfully merging this pull request may close these issues.

The pk secrets env command for meeting Development Environment Usecase
3 participants