search

MatrixAI / Polykey

Polykey Core Library
https://polykey.com
GNU General Public License v3.0
31 stars 4 forks source link

Updating behaviour of `VaultsSecretsGet` to align with requirements of `secrets cat` #805

Closed aryanjassal closed 1 month ago

aryanjassal commented 2 months ago

Description

The current VaultsSecretsGet only gets a single secret from the vault, and returns it in the form of a UnaryHandler, meaning for large files, the RPC call will timeout. It also fetches only one file at a time, so behaviour like UNIX's cat command isn't possible.

This PR aims to fix that issue by switching over the RPC handler to a ServerHandler, supporting larger files. This PR also adds support for listing the contents of multiple files in order, like what cat does.

Issues Fixed

Tasks

Final checklist

linear[bot] commented 2 months ago

ENG-356 Implement `secrets cat` command

aryanjassal commented 2 months ago

I have implemented basic functionality for displaying file contents for multiple files, and updated the tests in tests/client/handlers/vaults.test.ts.

I still need to review the issue spec and all the code to ensure everything's ready for review.

aryanjassal commented 2 months ago

https://github.com/MatrixAI/Polykey-CLI/issues/243#issuecomment-2348535798

I'm going to go ahead with the assumption that we need to implement support for listing contents for files from multiple vaults concurrently, as that makes sense and retains consistency with UNIX commands and secrets rm.

I will wait for review of my approach from secrets rm, and if my implementation seems correct, I will implement that in secrets cat too.

aryanjassal commented 2 months ago

The naming of secret commands has changed from what it was. The intention was to keep using the pattern of SecretsGet, SecretsPut, SecretsDel, but to better align with the new commands, these names have been changed to SecretsGet (I haven't changed this name yet), SecretsEdit, SecretsRemove.

We are kinda moving away from the pattern. This was hard to detect as this change was made one-at-a-time across multiple PRs. We might need to look into this and set a standard pattern to follow for ensuring consistency.

aryanjassal commented 2 months ago

While I am writing the tests, I can see that we use RPC calls everywhere, even when we don't need to. For example, if we are testing vaultsSecretsGet and vaultsSecretsRemove, then we will have RPC handlers for vaultsSecretsNew, vaultsSecretsMkdir, vaultsSecretsStat, etc. as other RPC handlers we use to help perform vault operations in the tests.

This introduces slowdowns as each RPC call takes time to complete. A faster method of doing the same would be to acquire a vault, and perform writeF on the vault filesystem directly. It should speed up all our tests.

@tegefaulkes and I had a chat about this, and he was also of the same opinion as I, but he mentioned how all the tests would need to be updated to ensure consistency.

How should I deal with this? Do I make a new issue tracking this? It should be relatively straightforward, so maybe this can be something @brynblack could attempt once she gets into Polykey Core development.

tegefaulkes commented 2 months ago

It's some thing we can address later. I think we should make a new issue for it. It should be a general review and refactor of the vaults domain tests. I think one for the CLI and one for Polykey. You mentioned that the vaults tests is one big test file. We should look into splitting that out again but we need to check if it still crashes the CI. The other part is dealing with the vaultOps.test.ts testfile. Currently its the longest running tests by far so if we can speed it up or split it into parts that would help as well.

tegefaulkes commented 2 months ago

Looks good to me.

aryanjassal commented 1 month ago

All tasks have been completed for this PR, all checks are passing, and reviews has also been approved. Merging.