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.

Sign up for GitHub

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

Jump to bottom

Using logger methods for agent shutdown messaging #328

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from
Open

Using logger methods for agent shutdown messaging #328

wants to merge 3 commits into from

Conversation

aryanjassal
Copy link
Contributor

@aryanjassal aryanjassal commented Nov 12, 2024 •
edited
Loading

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 Linear
Copy link

linear bot commented Nov 12, 2024

ENG-455 Add short or long form for commands where applicable

ENG-399 Use WARN logging for agent shutdown message

@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 •
edited
Loading

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 vaults pull
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 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aryanjassal aryanjassal

Labels
None yet
Milestone
No milestone
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
2 participants
@aryanjassal @CMCDragonkai