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

Implementing UNIX-like secrets ls command #255

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Aug 16, 2024

Description

This PR works on implementing UNIX-like secrets ls experience, as if one was ls-ing using the coreutils's ls command. As described in the issue spec, not all ls commands will be supported, only some major and useful ones.

Issues Fixed

Tasks

  • 1. Add UNIX-like ls support
  • 2. Confirm file filtering/globbing works
  • 3. Add options to render the contents using various flags
  • 4. Update all the test

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 Aug 16, 2024
Copy link

linear bot commented Aug 16, 2024

@aryanjassal
Copy link
Contributor Author

ls

This is the current state of the ls command. I'm not sure how to progress with the formatting of ls long output. The file permissions are redundant, and so are the users and groups. Instead of trying to retain perfect compatibility with UNIX commands, we will have to make a compromise.

For now, we need to remove permissions and users columns and fill it with something else. That is something we need to discuss. I have been thinking that we could do something like printing the vault ID instead of the permissions, and printing the type of file (directory or file) in place of the user and operators.

@CMCDragonkai
Copy link
Member

The metadata of ls is a bit redundant... given that it's always going to be the "root" user and "root" group. And permissions are always rwx for all.

So in a way we can cut down the number of irrelevant information while still preserving some interesting data. Like d for directories, file size and date time of last edit.

But other ideas is possible.

@CMCDragonkai
Copy link
Member

  1. I think we stick with simple ls showing individual files, one after another.
  2. Let's hold off on any options yet except maybe the long option..
  3. I feel like hidden files may need to be shown by default. No need for option.

@CMCDragonkai
Copy link
Member

The key thing here is that LS doesn't really need to match the Unix LS command. It's not that well defined anyway.

@CMCDragonkai
Copy link
Member

And more advanced features to LS can be added in later.

I want to see the other commands implemented as they are more important.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 19, 2024

I have noticed that globWalk works differently to how ls works. In ls, when any directories are listed, they are automatically entered and explored. However, in globWalk, that doesn't happen. I can understand that this decision makes sense, but this results in a lot of extra code to work around it. In this PR, I have to make a RPC call, then check if the result is a directory. If it is, then we need to make another RPC call with a different pattern specifically to explore the directory, which seems to me like a lot of unnecessary effort.

Moreover, by default, the 'glob' select (which is used internally to mean 'select everything') actually excludes hidden files, which is different to the behaviour in shell. This is what I have to current do to properly fetch all results, including hidden files, by default.

function formatPattern(pattern: string, directory: boolean = false): string {
function formatPattern(pattern: string, directory: boolean = false): string {
  if (pattern == '/') return '{*,.*}';
  if (pattern.charAt(0) == '/') pattern = pattern.substring(1);

  if (directory) pattern = `${pattern}/{*,.*}`;
  else pattern = `{*${pattern},.*${pattern}}`;
  return pattern;
}

The NPM page on minimatch mentioned that enabling the dot option would allow it to match filenames starting with ., but I haven't seen it work, so I will keep using this formatter for the time being.

I'm not sure if this is the best way to be handling this, so I would like @tegefaulkes to provide me feedback on this.

@aryanjassal
Copy link
Contributor Author

And more advanced features to LS can be added in later.
I want to see the other commands implemented as they are more important.

I am nearing completion on both the PRs in PK and PKCLI. The only major task that I have to do in MatrixAI/Polykey#785 is adding the second RPC handler for streaming file contents over RawHandler and trim the functionality of serializerStream to only handle serialising file contents.

@aryanjassal
Copy link
Contributor Author

If two options coincide with their short name, then the option which was registered first would be set. For example, if I have two options: '-l, --long' and '-l, --longer', then pass -l as a flag, I will see that only one of them will have the value of -l that was passed.

If the long option is added/registered first, then the value of -l will always update the long option. If the longer option is added/registered first, then longer will always be updated.

[aryanj@matrix-34xx]$ pk test flags
Options:
  -l --long            Use a long listing format (default: false)
  -l --longer          Use a longer format (default: false)

[aryanj@matrix-34xx]$ pk test flags -l
long: true
longer: false

[aryanj@matrix-34xx]$ pk test flags -l -l
long: true
longer: false

[aryanj@matrix-34xx]$ pk test flags -l --longer
long: true
longer: true

If both the short and long name are the same, then the second registered option will always be undefined.

[aryanj@matrix-34xx]$ pk test flags -l
long: true
another long: undefined

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Aug 19, 2024

Remember the discussion. We're not using the globWalk utility anymore. I think we can keep the functionality of that around for now but for now we don't need to implement it. That said, since globbing is a shell level construct, globbing normal fs paths will still work with the command so only the vault paths will not support it properly.

I think the we should at least stick to the format ls -l has so people can use our version interchangeably within their scripts. Same for the normal format. It should be simplified but I think each entry on a separate line would be more annoying to process than say, each entry separated by a space. That said, we could include the --zero and -1 option formats.

Lastly, when checking the conflicting short options. Remember that we have multiple level commands such as secrets being one command and secrets ls being a sub command. You need to check how the short options conflict across the levels of these commands. and if ordering of the option matters here. secrets -l ls vs secrets ls -l vs secrets -l ls -l.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 19, 2024

Remember the discussion. We're not using the globWalk utility anymore. [snip]

I thought that we weren't going to use the 'glob' feature of the globWalk utility, as otherwise, how am I supposed to get the list of the files along their stats from the file system?

I think the we should at least stick to the format ls -l has so people can use our version interchangeably within their scripts. [snip]

I also agree here, but it is true that most of that information is useless for us. We might be able to implement another flag which can use a newer formatting made for Polykey.

The ls utility also checks if it is running in interactive mode, and if it isn't then lists the files and directories one-by-one, otherwise lists them in the space-based format. I remember reading somewhere that we have checks for running in interactive mode, so we might be able to use them to implement this feature?

@aryanjassal
Copy link
Contributor Author

Lastly, when checking the conflicting short options. [snip]

There are no options for the secret sublevel. As such, I added the -l, --longer option in the CommandPolykey module, directly on the polykey command level.

The results were as expected. As -l, --longer was registered before ls's -l, --long, the root level -l --longer took precedence.

[aryanj@matrix-34xx]$ pk secrets ls
Options:
  -np, --node-path <path>      Path to Node State (default: "/home/aryanj/.local/share/polykey", env: PK_NODE_PATH)
  -pf, --password-file <path>  Path to Password
  -f, --format <format>        Output Format (choices: "human", "json", default: "human")
  -v, --verbose                Log Verbose Messages (default: 0)
  -l --longer                  longer (default: false)
  -ni, --node-id <id>           (env: PK_NODE_ID)
  -ch, --client-host <host>    Client Host Address (env: PK_CLIENT_HOST)
  -cp, --client-port <port>    Client Port (env: PK_CLIENT_PORT)
  -l --long                    Use a long listing format (default: false)
  -h, --help                   display help for command

[aryanj@matrix-34xx]$ pk secrets ls -l
long: false
longer: true
...

[aryanj@matrix-34xx]$ pk secrets -l ls
long: false
longer: true
...

[aryanj@matrix-34xx]$ pk -l secrets ls
long: false
longer: true
...

@tegefaulkes
Copy link
Contributor

I can't tell if you applied the two -l flags to secrets and secrets ls separately or both of them to secrets ls.

@aryanjassal
Copy link
Contributor Author

I can't tell if you applied the two -l flags to secrets and secrets ls separately or both of them to secrets ls.

The --longer flag was applied to the polykey namespace, as there were no flags in the secrets namespace. The --long flag has been applied only on the ls namespace.

[aryanj@matrix-34xx]$ pk --longer secrets ls
works

[aryanj@matrix-34xx]$ pk --long secrets ls
fails

[aryanj@matrix-34xx]$ pk secrets --long ls
fails

[aryanj@matrix-34xx]$ pk secrets --longer ls
works

[aryanj@matrix-34xx]$ pk secrets ls --longer
works

[aryanj@matrix-34xx]$ pk secrets ls --long
works

[aryanj@matrix-34xx]$ pk secrets ls --long --longer
works

@CMCDragonkai
Copy link
Member

Remember the discussion. We're not using the globWalk utility anymore. [snip]

I thought that we weren't going to use the 'glob' feature of the globWalk utility, as otherwise, how am I supposed to get the list of the files along their stats from the file system?

I think the we should at least stick to the format ls -l has so people can use our version interchangeably within their scripts. [snip]

I also agree here, but it is true that most of that information is useless for us. We might be able to implement another flag which can use a newer formatting made for Polykey.

The ls utility also checks if it is running in interactive mode, and if it isn't then lists the files and directories one-by-one, otherwise lists them in the space-based format. I remember reading somewhere that we have checks for running in interactive mode, so we might be able to use them to implement this feature?

It's not recommended for people to create scripts depending on ls. Like I said ls is not a standardized command. So regular ls should just do the simple thing which in our case is listing the contents one line at a time.

As for -l option, yea it could work for us to show more data. It should still use one of the standard output formatters we use though.

@CMCDragonkai
Copy link
Member

If two options coincide with their short name, then the option which was registered first would be set. For example, if I have two options: '-l, --long' and '-l, --longer', then pass -l as a flag, I will see that only one of them will have the value of -l that was passed.

If the long option is added/registered first, then the value of -l will always update the long option. If the longer option is added/registered first, then longer will always be updated.

[aryanj@matrix-34xx]$ pk test flags
Options:
  -l --long            Use a long listing format (default: false)
  -l --longer          Use a longer format (default: false)

[aryanj@matrix-34xx]$ pk test flags -l
long: true
longer: false

[aryanj@matrix-34xx]$ pk test flags -l -l
long: true
longer: false

[aryanj@matrix-34xx]$ pk test flags -l --longer
long: true
longer: true

If both the short and long name are the same, then the second registered option will always be undefined.

[aryanj@matrix-34xx]$ pk test flags -l
long: true
another long: undefined

So does this mean that if there is a global option like -l, this would conflict with our local option -l? At any case you then want to avoid any short or long options that is likely to be in conflict with any global option that currently has been defined on PK.

Again I would reduce the scope of this issue/pr to focus on the basic functionality first, not necessarily all the options of the Unix commands. It would take too long to complete otherwise.

@aryanjassal
Copy link
Contributor Author

So does this mean that if there is a global option like -l, this would conflict with our local option -l? At any case you then want to avoid any short or long options that is likely to be in conflict with any global option that currently has been defined on PK.
Again I would reduce the scope of this issue/pr to focus on the basic functionality first, not necessarily all the options of the Unix commands. It would take too long to complete otherwise.

Yes, if a global option and a local option both have conflicts, then the global one will take precedence.

I am working on the basic functionality, and that seems to be almost done. I can currently list the files in a vault, list files within a directory inside the vault, and list them in long format looking like this:

[aryanj@matrix-34xx]$ pk secrets ls myvault:/ -l
FILE  65 Aug 19 10:07 .hidden-file.txt
FILE 175 Jun 13 16:53 another.txt
FILE  54 Jun 13 16:53 newsecret.txt
FILE  17 Jun 13 16:51 secret1.txt
 DIR   0 Aug 19 12:21 test

[aryanj@matrix-34xx]$ pk secrets ls myvault:test -l
FILE 45822 Aug 19 12:21 test/.hidden-document.docx
FILE 34478 Aug 19 11:20 test/image.jpg
FILE  3364 Aug 19 11:19 test/something.txt

@tegefaulkes
Copy link
Contributor

BTW, GlobWalk just walks the tree based on a glob pattern. It isn't an implementation for LS. So if it only matches the directory it will only output the directory and not its contents.

@CMCDragonkai
Copy link
Member

I prefer a suffix of / for directories and drop the entire FILE vs DIR column.

@CMCDragonkai
Copy link
Member

I wonder would the hardlink count be useful.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 19, 2024

I prefer a suffix of / for directories and drop the entire FILE vs DIR column.

Now, the output of the long list looks like this:

[aryanj@matrix-34xx]$ pk secrets list myvault:/ -l
  65 Aug 19 10:07 .rippling-dns-record.txt
 175 Jun 13 16:53 another.txt
  54 Jun 13 16:53 newsecret.txt
  17 Jun 13 16:51 secret1.txt
   - Aug 19 12:21 test/

[aryanj@matrix-34xx]$ pk secrets list myvault:/ -lh
Size Date Modified Path
  65 Aug 19 10:07  .rippling-dns-record.txt
 175 Jun 13 16:53  another.txt
  54 Jun 13 16:53  newsecret.txt
  17 Jun 13 16:51  secret1.txt
   - Aug 19 12:21  test/

@CMCDragonkai
Copy link
Member

Let's add back in the hardlink counts.

Yes, if a global option and a local option both have conflicts, then the global one will take precedence.

If this is true, then how is your -lh option working? Would't the short option of -h in the local case conflict with the -h of the global help option?

I'm basically pointing out here, that if the global option takes precedence, you cannot have any conflicting local options.

Global options stem from the CommandPolykey. Local options are from the individual subcommand.

@CMCDragonkai
Copy link
Member

BTW if you're doing prefix offsets, that means you are buffering the output in the case of the long option?

@CMCDragonkai
Copy link
Member

Here is the ACTUAL posix spec of ls: https://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html.

Have a read of the details there.

The ls that is in your terminal is GNU ls which isn't necessarily the stasndard https://unix.stackexchange.com/questions/754981/what-are-non-gnu-versions-of-terminal-commands.

@CMCDragonkai
Copy link
Member

d
Directory.
b
Block special file.
c
Character special file.
l (ell)
Symbolic link.
p
FIFO.
-
Regular file

Those are the entry type characters. In EFS, I don't believe we have block and character files. But we do have symlinks.

So...

d
l
-

Can be used.

@CMCDragonkai
Copy link
Member

The default format shall be to list one entry per line to standard output; the exceptions are to terminals or when one of the -C, [XSI] [Option Start] -m, or -x [Option End] options is specified. If the output is to a terminal, the format is implementation-defined.

@CMCDragonkai
Copy link
Member

If no operands are specified, ls shall write the contents of the current directory. If more than one operand is specified, ls shall write non-directory operands first; it shall sort directory and non-directory operands separately according to the collating sequence in the current locale.

That does imply buffering up to sort by alphanum. We would just assume english. But technically unix paths are allowed any character except /. However I believe in the case of PK , would limit to a common set of characters available to all OS including windows limitations.

Would we then use LANG and other things for sorting? That doesn't even exist on Windows, so I guess we would just sort alphanum for now. Whatever JS does with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Aug 19, 2024

If the -l option is specified without -L, the following information shall be written:

"%s %u %s %s %u %s %s\n", , ,
, , ,
,

So I would do something like this:

d 3 vair3u8kqfqoheulk277bp71j5vi80vd2bnavki02ekeounp72e80  11 Aug 15 15:22 tmp/
- 1 vair3u8kqfqoheulk277bp71j5vi80vd2bnavki02ekeounp72e80 946 May  5  2023 ADDITIONAL_TERMS
l 1 vair3u8kqfqoheulk277bp71j5vi80vd2bnavki02ekeounp72e80  71 May 10 21:13 result -> /nix/store

What do you think?

Maybe even <nodeId> is superfluous (because it's always going to be the same node ID), in which case:

d 3  11 Aug 15 15:22 tmp/
- 1 946 May  5  2023 ADDITIONAL_TERMS
l 1  71 May 10 21:13 result -> /nix/store

It's just the lack of separation between hardlinks and the file size is a bit difficult to decipher.

Without hardlinks...

d  11 Aug 15 15:22 tmp/
- 946 May  5  2023 ADDITIONAL_TERMS
l  71 May 10 21:13 result -> /nix/store

Hardlink information - primarily indicates whether the file data is going to be deleted. It is useful information as it will even tell you how many files in the directory.

@CMCDragonkai
Copy link
Member

Take note that the date time showing, changes dpeending on whether it is a regular time date vs year.

I think also it is localized against the local timezone too. So by default the files contain the UTC timezone, you'd want to localise that time date too.

@CMCDragonkai
Copy link
Member

Due to both alphabetical ordering AND prefix offsetting for formatting, that means the long format is the same as the table output formatter... However I believe they use space character, not tab character, so I guess it's not a table output formatter.

Either both styles involve buffering. It's not a stream based listing command, unless we implement secrets find (which we could...).

@CMCDragonkai
Copy link
Member

Oh and the / suffix for directories should only occur for the long mode. Not for regular ls, since we would want to make sure that when piping, any commands that make use of ls output should expect that all content entries are just the name of the file and nothing else.

@CMCDragonkai
Copy link
Member

In your tests, you should also test for possiblities of filenames as per https://stackoverflow.com/a/31976060. As well as dealing with potential newlines within a file name. And how the ls would show that too. We may need to do escaping.

For example if a file name is a b c, I believe, ls will show 'a b c' with the single quotes.

Furthermore \n would get quoted too so a\nb would be 'a\nb'.

So single quotation would occur for some whitespace characters.

However a®b stays a®b.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 20, 2024

If this is true, then how is your -lh option working? Would't the short option of -h in the local case conflict with the -h of the global help option?

Most likely because -h is defined by commander, and the reason I was able to override it is probably to allow for the user to override displaying the help menu. I will change it, then.

BTW if you're doing prefix offsets, that means you are buffering the output in the case of the long option?

Yes, I am buffering the data by adding it to an array to apply further formatting to it. I haven't currently sorted the entries, just storing them in their iNode order and listing them in same.

Oh and the / suffix for directories should only occur for the long mode. Not for regular ls, since we would want to make sure that when piping, any commands that make use of ls output should expect that all content entries are just the name of the file and nothing else.

Yes, this is how it is handled currently. However, there is one catch. If we want to list the contents of the root directory, we do:

[aryanj@matrix-34xx]$ pk secrets ls newvault:
[wrong command]

[aryanj@matrix-34xx]$ pk secrets ls newvault:/
/.hidden-file.txt
/another.txt
/newsecret.txt
/secret1.txt
/test

[aryanj@matrix-34xx]$ pk secrets ls newvault:/test
/test/.document.docx
/test/image.jpg
/test/something.txt

[aryanj@matrix-34xx]$ pk secrets ls newvault:test
test/.document.docx
test/image.jpg
test/something.txt

Basically, leading slashes changes the output of the listing, and I'm not sure if this needs to change, or not.

It's just the lack of separation between hardlinks and the file size is a bit difficult to decipher.

That is why I added an option to display the header in long formatting, to make it easier to understand. Maybe I can enable it by default?

Take note that the date time showing, changes dpeending on whether it is a regular time date vs year.
I think also it is localized against the local timezone too. So by default the files contain the UTC timezone, you'd want to localise that time date too.

By default, the date data is stored in milliseconds since 1 Jan 1970. I am currently using JavaScript's date utility, which automatically performs the conversion of the date based on the timezone of the location where it was called from.

n your tests, you should also test for possiblities of filenames as per https://stackoverflow.com/a/31976060. As well as dealing with potential newlines within a file name. And how the ls would show that too. We may need to do escaping.

I haven't done this bit yet, but shouldn't be too difficult to implement.

@aryanjassal
Copy link
Contributor Author

However I believe they use space character, not tab character, so I guess it's not a table output formatter.

At this point, I'm basically writing a table formatter, but replacing the tab character with a space. Maybe, instead of doing that, I can simply modify the outputFormatterTable to optionally take a custom separator, defaulting to \t.

@CMCDragonkai
Copy link
Member

BTW if you're doing prefix offsets, that means you are buffering the output in the case of the long option?

Yes, I am buffering the data by adding it to an array to apply further formatting to it. I haven't currently sorted the entries, just storing them in their iNode order and listing them in same.

Yes I'm just changing my tune here and saying that we can buffer it up and sort it.

Oh and the / suffix for directories should only occur for the long mode. Not for regular ls, since we would want to make sure that when piping, any commands that make use of ls output should expect that all content entries are just the name of the file and nothing else.

Yes, this is how it is handled currently. However, there is one catch. If we want to list the contents of the root directory, we do:

[aryanj@matrix-34xx]$ pk secrets ls newvault:
[wrong command]

[aryanj@matrix-34xx]$ pk secrets ls newvault:/
/.hidden-file.txt
/another.txt
/newsecret.txt
/secret1.txt
/test

[aryanj@matrix-34xx]$ pk secrets ls newvault:/test
/test/.document.docx
/test/image.jpg
/test/something.txt

[aryanj@matrix-34xx]$ pk secrets ls newvault:test
test/.document.docx
test/image.jpg
test/something.txt

No that's not what I meant here. I'm saying long mode in the sense of the -l option for the output of the command. And you should still enable the ability to do ls newvault and ls newvault:/ simultaneously and newvault: should be an error.

@CMCDragonkai
Copy link
Member

However I believe they use space character, not tab character, so I guess it's not a table output formatter.

At this point, I'm basically writing a table formatter, but replacing the tab character with a space. Maybe, instead of doing that, I can simply modify the outputFormatterTable to optionally take a custom separator, defaulting to \t.

No that won't work, the table formatter should be using tab as tab spacing in the terminal fixes the tab width.

However I noticed that ls itself might actually be using spaces directly. So in the long mode, you'd format it similarly using the list format but with the appropriate spaces. See you have to find the longest field of each column and prefix accordingly if you want to match the exact same style.

You could have a different table output formatter thie time call it outputFormatterTableWithSpaces that does the calculation.

@aryanjassal
Copy link
Contributor Author

No that won't work, the table formatter should be using tab as tab spacing in the terminal fixes the tab width.

In the latest commit, I actually implemented this already. So far, nothing has broken, and everything works as intended.

This is the output that the table formatter gives:

[aryanj@matrix-34xx]$ pk secrets ls newvault:/ -lH
Type Size Links Date Modified Name
-    1    65    Aug 19 10:07  .rippling-dns-record.txt
-    1    175   Jun 13 16:53  another.txt
-    1    54    Jun 13 16:53  newsecret.txt
-    1    17    Jun 13 16:51  secret1.txt
d    2    0     Aug 19 12:21  test/

[aryanj@matrix-34xx]$ pk secrets ls newvault:/ -l
- 1 65  Aug 19 10:07 .rippling-dns-record.txt
- 1 175 Jun 13 16:53 another.txt
- 1 54  Jun 13 16:53 newsecret.txt
- 1 17  Jun 13 16:51 secret1.txt
d 2 0   Aug 19 12:21 test/

@aryanjassal
Copy link
Contributor Author

I have noticed two problems with how the table formatter formats the ls output. Firstly, ls outputs contents with padding applied to the beginning, but the table formatter does so with the padding applied to the end.

[aryanj@matrix-34xx]$ pk secrets ls newvault -l
- 1 65  Aug 19 10:07 .hidden-file.txt
- 1 175 Jun 13 16:53 another.txt
- 1 54  Jun 13 16:53 longer secret.txt
- 1 17  Jun 13 16:51 secret1.txt
d 2 0   Aug 19 12:21 test/

[aryanj@matrix-34xx]$ # What the correct version would output
[aryanj@matrix-34xx]$ pk secrets ls newvault -l
- 1  65 Aug 19 10:07 .hidden-file.txt
- 1 175 Jun 13 16:53 another.txt
- 1  54 Jun 13 16:53 longer secret.txt
- 1  17 Jun 13 16:51 secret1.txt
d 2   0 Aug 19 12:21 test/

I will add an option to the table formatter to change the text alignment from left aligned to right aligned.

The second issue I had was the auto-escaping done by the table formatter. It would escape things like quotations, but not space. For example, it would render filename with spaces.txt as-is, but if I try to manually insert the quotations, it would be escaped like this: "\'filename with spaces.txt\'".

My first idea was to simply have an option to override the regex and the replacer, but that seemed too janky, so Brian helped me come up with another solution. Instead of overriding the escape function and regex, I can just disable it, and provide it with the padded strings by myself. The disabling of the escaping function will again be an option in the table formatter.

I need to use the table formatter, as the output of ls is basically covered by a table formatter, but with minor changes. Instead of rewriting an entire formatter in the command just to change a line or two of code, I can instead use and extend the table formatter instead, making my own code easier to write and providing extra functionality should it be needed in the future for any reasons.

@aryanjassal
Copy link
Contributor Author

I will add an option to the table formatter to change the text alignment from left aligned to right aligned.

This actually does not work.

[aryanj@matrix-34xx]$ pk secrets ls newvault -l
- 1  65 Aug 19 10:07  .hidden-file.txt
- 1 175 Jun 13 16:53       another.txt
- 1  54 Jun 13 16:53 longer secret.txt
- 1  17 Jun 13 16:51       secret1.txt
d 2   0 Aug 19 12:21             test/

It also aligns the files to the right, not just the text like hard links, block sizes, etc. basically any field which can have a variable length. Thus, the easiest solution right now appears to create a utility function which will apply padding for any field manually, then manually padding the hard links and file size field. Or, alternatively, just ignoring this issue and keeping everything left-aligned.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 23, 2024

  1. Support specifying multiple paths.
  2. Seamlessly support using normal and secret paths.

For 6., it would be slightly more complex supporting that as we would need to perform multiple RPC calls and somehow handle displaying them all. This would especially be an issue as this style would be inherently incompatible with how the table formatter works, so this would force me to basically write a custom formatter in the ls command handler, which seems needlessly excessive.

For 7., I don't think supporting it is a good idea. We are only displaying information for the files within the vault, and only the information which is relevant to it. The filesystem would have more information than the information we show for vault files.

Moreover, I support syntax where you can just specify the vault name and all the files in the vault are automatically listed out. If we do end up implementing this functionality, then it would break this syntax.

[aryanj@matrix-34xx]$ pk secrets ls newvault

My suggestion is to let the user use their own system utilities to interact with their local file system. For example, scripts made in windows relying on its dir utility will break if they replace the command with Polykey's ls command. Even the scripts written on linux using GNU ls will break as we don't follow the same layout, so I don't think we should display system files too.

Brian's argument was that we would need to seamlessly interact with the user's file system for commands like cp, but we already do that when we are using pk secrets create. So, overall, I think these last two points need to be thought about a bit more.

@tegefaulkes @CMCDragonkai

@tegefaulkes
Copy link
Contributor

The entire point of #32 is for it to function seamlessly like the unix commands. To do that both 6. and 7. are requirements for this.

Specifying multiple paths is pretty essential to the LS command. In fact, its why it works at all with the glob expansion the shell does. But the main thing is, without using the globWalk utility to specify a globstar pattern we have not way of outputting multiple files unless we specify multiple paths. Making multiple RPC calls shouldn't be a problem here. It should be expected at this stage.

  1. is an essential feature of the other unix style commands. we should include it here if only to make all of the new commands consistent.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Aug 23, 2024

As per our discussion, I am removing the extended features included in this PR, including the detailed view, globbing, etc, as those features aren't high on the priority list now.

@aryanjassal aryanjassal marked this pull request as ready for review August 23, 2024 09:35
src/secrets/CommandList.ts Outdated Show resolved Hide resolved
src/secrets/CommandList.ts Outdated Show resolved Hide resolved
src/secrets/CommandList.ts Outdated Show resolved Hide resolved
src/utils/parsers.ts Outdated Show resolved Hide resolved
tests/secrets/list.test.ts Outdated Show resolved Hide resolved
aryanjassal and others added 3 commits August 30, 2024 13:41
wip: working on adding unix-like ls support

feat: added ls for listing vaults with long and short formatting

[ci skip]

feat: added option to show hidden files and removed redundant metadata

[ci skip]

feat: added directory traversal, showing hidden files by default

[ci skip]

chore: simplified pattern formatting code

[ci skip]

chore: updated long list formatting and added header option

[ci skip]

chore: updated long list formatting and header formatting

[ci skip]

chore: updated date formatter to handle old files

chore: removed redundant directory rpc call and removed leading slashes from file paths

[ci skip]

chore: added error handling for invalid patterns

[ci skip]

chore: updated long list formatting using table formatter

[ci skip]

chore: added new parser for parsing secret filepaths

[ci skip]

chore: simplifying functionality for merging

[ci skip]

chore: cleaning up code

[ci skip]

chore: fixed tests

chore: consolidating redundant maps

fix: lint

chore: updated tests and added edge cases

fix: lint
[ci skip]

chore: updated parsers
[ci skip]
@aryanjassal
Copy link
Contributor Author

Brian helped me with some minor issues and cleaning up the code for the parsers. This PR now looks ready. Merging.

@aryanjassal aryanjassal merged commit 45edcf3 into staging Aug 30, 2024
22 checks passed
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