Skip to content

Conversation

@aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Nov 12, 2024

Description

Currently, the status of the logger is directly written to the standard error using proces.stderr.write(), but that is not ideal. We should be using logger for all messaging and communication.

This PR changes the 'Stopping Agent' message when stopping the agent from writing to standard error to using logger to do it.

As #323 is a small change, it will be done alongside this PR, too.

Issues Fixed

Tasks

  • 1. Remove process.stderr.write() and replace it with logger.warn
  • 2. All the commands which should have a short/long form must have it. For example, vaults rm|remove, etc.

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 Nov 12, 2024
@linear
Copy link

linear bot commented Nov 12, 2024

@aryanjassal
Copy link
Contributor Author

I've gone through all the matches for process.stderr.write and the only places where it has been used are:

  1. To write the output of an output formatter
  2. To provide interactive feedback to the user (or any other thing meant to be seen by the user)
  3. All secrets commands which can continue on error

@aryanjassal
Copy link
Contributor Author

Currently, the command for deleting a vault is vaults delete. To make it consistent with secrets commands, I can alias it as vaults rm and vaults remove. Should I also alias the command as vaults del for being the short form of the word 'delete'?

@CMCDragonkai @tegefaulkes

@CMCDragonkai
Copy link
Member

No delete should be removed.

@aryanjassal
Copy link
Contributor Author

No delete should be removed.

Okay, so I need to rename vaults delete to vaults rm|remove. Got it.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Nov 12, 2024

Vaults commands

Name Command Alias
Clone a vault vaults clone N.A.
Create a vault vaults create vaults touch
Deletes a vault vaults rm vaults remove
Lists all vaults vaults ls vaults list
Show vault log vaults log N.A.
Vault permissions vaults permissions vaults perms
Pull a vault vaults pull N.A.
Rename a vault vaults rename N.A.
Scan a node vaults scan N.A
Share a vault vaults share N.A.
Unshare a vault vaults unshare N.A.
Get the vault version vaults version N.A.

Secrets commands

Name Command Alias
Concatenate a secret secrets cat N.A.
Copy a secret from disk secrets create N.A.
Copy a directory from disk secrets dir N.A.
Edit a secret with editor secrets edit secrets ed
Export vault as env secrets env N.A.
List secrets secrets ls secrets list
Make a directory secrets mkdir N.A.
Rename a secret secrets rename N.A.
Stat a secret secrets stat N.A
Write secret from stdin secrets write N.A.

This is the current state of secrets and vaults commands that we have as of this PR. Can any other command be changed to support other aliases, or is what we have sufficient to consider all commands that can be Unix-ified have now been Unix-ified?

@CMCDragonkai @tegefaulkes

@aryanjassal aryanjassal marked this pull request as ready for review November 12, 2024 23:50
@CMCDragonkai
Copy link
Member

Don't know about vaults touch. That doesn't need to exist. Create is ok.

@CMCDragonkai
Copy link
Member

What about secrets install too and secrets cp and secrets mv.

@aryanjassal
Copy link
Contributor Author

aryanjassal commented Nov 13, 2024

What about secrets install too and secrets cp and secrets mv.

Those commands have not been created yet, as I am now focusing on critical bugs instead of adding more secrets commands.

But if they were added, they would like like so:

Name Command Alias
Secrets install secrets install N.A.
Copy a secret secrets cp secrets copy
Move a secret secrets mv secrets move

But this will be done when the relevant commands get created, and is not relevant for this PR.

@aryanjassal
Copy link
Contributor Author

Should vaults rename be aliased to vaults mv|move to be consistent with the secrets command?

@CMCDragonkai
Copy link
Member

No because it's not being moved anywhere.

@aryanjassal aryanjassal force-pushed the feature-warn-agent-shutdown branch from ff52b77 to e4dbc27 Compare November 18, 2024 02:00
chore: change command for vaults delete to be rm|remove

fix: updated tests

fix: removed touch alias for vaults create

fix: updated tests
@aryanjassal aryanjassal force-pushed the feature-warn-agent-shutdown branch from e4dbc27 to bd34c5c Compare November 18, 2024 02:08
Copy link
Contributor Author

@aryanjassal aryanjassal left a comment

Choose a reason for hiding this comment

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

Everything looks good, and all the requested changes have been made in this PR. All the tests also pass, so this PR is ready. Merging.

@aryanjassal aryanjassal merged commit 7ce7f16 into staging Nov 18, 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.

Add short or long form for commands where applicable Use WARN logging for agent shutdown message

3 participants