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

Allow mkdir command to create multiple directories #300

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Oct 4, 2024

Description

The current mkdir implementation only supports creating a single directory at a time. This needs to be expanded to work similar to UNIX's mkdir command, which can make multiple directories at the same time, and does not stop when it encounters an error.

This PR implements similar features to secrets mkdir command, allowing the creation of multiple directories across vaults in a single command.

Note: as per Polykey#819 (comment), a warning or notice needs to be sent to stderr, warning users that the directory, while it is created, is not part of the source tree and a file must exist within the directory in order for the directory to be detected as a part of the source tree.

Issues Fixed

Tasks

  • 1. Update secrets mkdir to align with UNIX mkdir
  • 2. Update tests for secrets mkdir
  • 3. Add warning informing users about empty directories being out-of-tree

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 Oct 4, 2024
Copy link

linear bot commented Oct 4, 2024

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Oct 4, 2024

How do I handle sending the warning to the user? Should a random warning message be simply sent to stderr like this? Is the amount of information sufficient, or does it need rewording?

[aryanj@matrix-34xx:~]$ pk secrets mkdir vault:dir
WARNING: Empty directories will not be affected by modifications to the vault state (version change, sharing/cloning, etc.).

It seems a bit random how secrets rm, secrets cat, and now secrets mkdir all have their own versions of error/warning messages with no standard.

Like I discussed in Polykey#819, it would probably be better if instead of passing an error message, we instead return an object containing details of the error (serialising the error will probably take up too much bandwidth for minimal to no return), which can then be interpreted by the client (CLI) to adapt to their own needs. For example, when we make a Polykey Desktop, then the error messages we have are designed for UNIX CLI and would feel awkward on other clients.

Instead of relying on UNIX's behaviour and messages, which itself doesn't follow a strict standard, we should devise our own standard of printing warnings and errors. This is different to the formatters, as the formatters will format a certain type of output in a certain way. We, however, need a standard of message content.

We also need a standard way to display these warnings/errors. It could be made using a new formatter, it could be made using the logger. It doesn't matter how we get consistency, but I'd say consistency in user experience is something pretty important.

@tegefaulkes
Copy link
Contributor

Yeah we'll want a standard way of dealing with this but it doesn't need to be a fully serialized version of the Error object itself. Create a standard error message under the client types and have all rm, cat and mkdir conform to this structure when sending the details of the error. This is only for expected errors from the FS. If it was an unexpected error the we still throw it using the normal methods.

You'll need a way to tell the errors from the normal messages. for this you'd use a typed object for pattern matching the messages.

So we'd have

type Message = {
 //...whatever
}

type ErrorMessage = {
 type: 'error',
//...whatever
}

type HandlerMessage = Message | ErrorMessage;

So typescript is smart enough to tell the two apart. You can pattern match for one or the other implcity by checking if message.type === 'error'. If that is true then we know it's a error message otherwise its a normal message.

if (message.type === 'error') {
   // message will by typed as a ErrorMessage here implicitly.
   } else {
      // message is now a Message here.
   }

Use this to tell them apart on the client side and construct our formatted message. As for what to make the error message. Just include the minimal details of the error we need to know. Message, error name, code and exit code if applicable.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Oct 8, 2024

So typescript is smart enough to tell the two apart. You can pattern match for one or the other implcity by checking if message.type === 'error'. If that is true then we know it's a error message otherwise its a normal message.

I implemented this here, Polykey#819, but I ran into an interesting issue.

Typescript doesn't actually quite work like this. If only one object has the type attribute, then typescript will complain of a missing attribute. As such, both types need to feature a common attribute for it to work like this.

As SuccessMessage is a single boolean, and it is used everywhere, I chose to add the success type to ErrorMessage instead, to push up a proof-of-concept for this. The PoC is working as intended, so we now need to decide what to do with the common attribute set. Do we have success as the common attribute, or type?

In my opinion, having type would make sense, but a lot of code would need to change for that. Or, we can have a SuccessTypeMessage and ErrorTypeMessage to differentiate the { success: boolean; } from { type: 'success'; success: boolean; }.

Need some thoughts on this @tegefaulkes.

@tegefaulkes
Copy link
Contributor

In the example I gave I figured you could drop the type parameter for one of the messages and still pattern match on that. In that case you'd have to check if the type parameter actually exists which is a little more annoying. So if both messages have the type defined then you can just pattern match on that and get the implicit type reduction.

Let's stick with type then. But would a lot a code change due to this? As far as I can tell it would only affect the response messages of the secret command handlers that need it. And obviously their CLI code using it but that's not a huge change.

@aryanjassal
Copy link
Contributor Author

Let's stick with type then. But would a lot a code change due to this? As far as I can tell it would only affect the response messages of the secret command handlers that need it. And obviously their CLI code using it but that's not a huge change.

Yes, a couple of handlers use this and a couple places in the CLI too. I still think that there could be two success messages. One is the SuccessMessage that we are already using, and other is the SuccessTypeMessage which could have the type attached to it, however it probably would result in code duplication and redundancy.

As such, I will make the relevant changes across Polykey's handlers and CLI's relevant commands and tests.

src/secrets/CommandMkdir.ts Outdated Show resolved Hide resolved
tests/secrets/mkdir.test.ts Outdated Show resolved Hide resolved
tests/secrets/mkdir.test.ts Outdated Show resolved Hide resolved
@aryanjassal aryanjassal marked this pull request as ready for review October 14, 2024 03:41
src/secrets/CommandMkdir.ts Outdated Show resolved Hide resolved
src/secrets/CommandMkdir.ts Outdated Show resolved Hide resolved
src/secrets/CommandMkdir.ts Outdated Show resolved Hide resolved
src/secrets/CommandMkdir.ts Outdated Show resolved Hide resolved
chore: updated tests for mkdir

chore: updated mkdir and added tests

chore: added warning for untracked directory

fix: removed write.yml

chore: working on adding non-zero exit codes

chore: updated tests to be more accurate

chore: addressed review

fix: manually updated type of error

chore: simplified streaming paths to handler

chore: updated polykey version

fix: lint

fix: updated tests

fix: lint
@aryanjassal
Copy link
Contributor Author

All the tasks have been done in this PR and the CI is also passing. Merging.

@aryanjassal aryanjassal merged commit 4a75142 into staging Oct 15, 2024
22 checks passed
@aryanjassal
Copy link
Contributor Author

mkdir
pkcli-mkdir.yml

@CMCDragonkai
Copy link
Member

mkdir mkdir pkcli-mkdir.yml

That is good, but why the background so large? And is there a way to scroll the terminal? Also speed up the actual typing. It needs to be done quickly.

@CMCDragonkai
Copy link
Member

Also the textual resolution looks worse than ascinnema...

@aryanjassal
Copy link
Contributor Author

That is good, but why the background so large? And is there a way to scroll the terminal? Also speed up the actual typing. It needs to be done quickly.

The background is so large so we can crop it to a fitting size later on, as re-rendering it will take some time.

There is a way to scroll the terminal. It just used by actual terminal size by default, as I was recording in a vertically split terminal. I can modify the rows and columns in the recording and can change it pretty easily.

As for speed, there should definitely be a way to do that. I will have to look into how to get that working though.

Also the textual resolution looks worse than ascinnema...

Probably because its so zoomed out. I will experiment with settings and upload a functional config.yml file on MatrixAI-Graph#79

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