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

Adding secrets cat command and removing secrets get command #282

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Sep 16, 2024

Description

This PR tracks the status of implementing secrets cat command, which can be used to concatenate multiple secrets together.

Issues Fixed

Tasks

  • 1. Add secrets cat command
  • 2. Remove secrets get command
  • 3. Add support for concatenating multiple files from multiple vaults together
  • 4. Redirect stdin to stdout if no arguments are provided
  • 5. Add tests for new functionality

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@aryanjassal aryanjassal self-assigned this Sep 16, 2024
Copy link

linear bot commented Sep 16, 2024

@aryanjassal aryanjassal force-pushed the feature-unix-cat branch 2 times, most recently from 1961d58 to 24bcc4f Compare September 20, 2024 02:07
@aryanjassal aryanjassal marked this pull request as ready for review September 20, 2024 02:07
Copy link
Contributor

@tegefaulkes tegefaulkes left a comment

Choose a reason for hiding this comment

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

Some changes needed. I made some comments but also address what we discussed.

src/secrets/CommandCat.ts Outdated Show resolved Hide resolved
src/secrets/CommandCat.ts Outdated Show resolved Hide resolved
src/secrets/CommandCat.ts Outdated Show resolved Hide resolved
src/secrets/CommandRemove.ts Outdated Show resolved Hide resolved
tests/secrets/cat.test.ts Outdated Show resolved Hide resolved
src/secrets/CommandCat.ts Outdated Show resolved Hide resolved
tests/secrets/cat.test.ts Show resolved Hide resolved
@tegefaulkes
Copy link
Contributor

Weird, I replied to your question as part of a new review so it posted it twice.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Sep 24, 2024

cat in UNIX, when being piped into another command, buffers the contents internally, accumulating stdout. Any errors written to stderr are printed immediately, and stdout is printed after all the files have been fully read. This results in the following output:

[aryanj@matrix-34xx:~]$ cat file1 nofile file2
content1
cat: nofile: No such file or directory
content2

[aryanj@matrix-34xx:~]$ cat file1 nofile file2 | cat
cat: nofile: No such file or directory
content1
content2

This implementation directly streams contents to stdout as we receive them. This results in the output being as follows:

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2
content1
ErrorSecretsSecretUndefined: nofile: No such file or directory
content2

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2 | cat
content1
ErrorSecretsSecretUndefined: nofile: No such file or directory
content2

It looks like the command is not doing anything, but if we redirect stderr to null in both cases, we get this:

[aryanj@matrix-34xx:~]$ cat file1 nofile file2 2> /dev/null
content1
content2

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2 2> /dev/null
content1
content2

We currently don't detect if piping is being done or not, and we don't have a way of replicating this behaviour.

@CMCDragonkai
Copy link
Member

cat in UNIX, when being piped into another command, buffers the contents internally, accumulating stdout. Any errors written to stderr are printed immediately, and stdout is printed after all the files have been fully read. This results in the following output:

[aryanj@matrix-34xx:~]$ cat file1 nofile file2
content1
cat: nofile: No such file or directory
content2

[aryanj@matrix-34xx:~]$ cat file1 nofile file2 | cat
cat: nofile: No such file or directory
content1
content2

This implementation directly streams contents to stdout as we receive them. This results in the output being as follows:

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2
content1
ErrorSecretsSecretUndefined: nofile: No such file or directory
content2

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2 | cat
content1
ErrorSecretsSecretUndefined: nofile: No such file or directory
content2

It looks like the command is not doing anything, but if we redirect stderr to null in both cases, we get this:

[aryanj@matrix-34xx:~]$ cat file1 nofile file2 2> /dev/null
content1
content2

[aryanj@matrix-34xx:~]$ polykey secrets cat vault:file1 vault:nofile vault:file2 2> /dev/null
content1
content2

We currently don't detect if piping is being done or not, and we don't have a way of replicating this behaviour.

  1. The pipe buffering is still streamed, it's just dependent on chunk size.
  2. You can detect if you are composed but that's a CLI concern not the PK agent concern.
  3. Leave the pipe buffering to the CLI - it's done by default by the OS. You don't need to do it, you just switch modes.
  4. Between agent to client you should still be streaming regardless of what the CLI is doing later.

@aryanjassal
Copy link
Contributor Author

Between agent to client you should still be streaming regardless of what the CLI is doing later.

Yes, that is what I am currently doing. Brian mentioned writing the streamed contents we receive into a temporary file, and then stream the file's contents to stdout, but streaming the incoming data straight to stdout was much simpler and faster.

@aryanjassal aryanjassal force-pushed the feature-unix-cat branch 2 times, most recently from 0cb0d17 to 23f872d Compare September 24, 2024 06:22
feat: removed Secrets/CommandGet and added tests

chore: working on stdin test

bug: working on adding test for stdin to stdout

bug: still working on that error

chore: fixed stdin test

feat: updated secrets edit with new secrets get RPC

fix: build

chore: updated way of specifying multiple files

chore: simplified tests

chore: undefined files are gracefully ignored

chore: updated implementation to use metadata flags

chore: version bump

fix: tests

fix: package lock

fix: npmdepshash
@tegefaulkes
Copy link
Contributor

Try not to mix up what I said.

#282 (comment)

@aryanjassal
Copy link
Contributor Author

All the checks have been passed, all the tasks have been finished, and the PR has also been approved. Merging.

@aryanjassal aryanjassal merged commit 165c9f4 into staging Sep 24, 2024
22 checks passed
@aryanjassal
Copy link
Contributor Author

cat

@CMCDragonkai
Copy link
Member

cat cat

Is it possible for you to disable your autosuggest before you record.

@aryanjassal
Copy link
Contributor Author

Yes. I can actually edit the recording itself to remove the captured autosuggest lines. I will do this and upload the new recording when I next work.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Sep 27, 2024

cat

I have edited the GIF to not include autocompletion anymore. I have also sped it up a little bit. @CMCDragonkai

I will be attaching the raw .cast file for reference and archival purposes. The file will be attached as a text file as GitHub doesn't support attaching files with arbitrary extensions.

cat.cast

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.

3 participants