-
Notifications
You must be signed in to change notification settings - Fork 3
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
List secrets with last updated, input bugfix, create/update instructions update #177
Conversation
Codecov Report
@@ Coverage Diff @@
## main #177 +/- ##
=====================================
Coverage 6.22% 6.22%
=====================================
Files 5 5
Lines 257 257
=====================================
Hits 16 16
Misses 241 241 |
@@ -11,14 +11,16 @@ import ( | |||
var dataFile string | |||
|
|||
func readInputConfig() { | |||
viper.SetConfigType("yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC -f filename.yaml
wasn't working until I moved this up here from outside of the case "-"
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't think of a reason for this to not be applied in certain conditions vs others, so it makes sense to move to the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is explicitly set to yaml
below since viper
has no way to infer the filetype from the stdin input.
viper.SetConfigFile() defines the path, name and extension of the config file. Then viper.ReadInConfig()
will know how to read it later.
If somebody is using a .json
file I would assume viper would internally reset the configType below?
In any case, @rocktavious has mentioned that we should look into a better solution for reading user input in place of viper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya we need to fix this reading from input problems globally
switch dataFile { | ||
case ".": | ||
// TODO: does this block ever actually ever run? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find a way to get it to trigger though, at least on bash/zsh. IIRC I ran into this problem before, passing .
as an argument in CLI's is a bad idea 😅
I don't think we should remove it immediately, but we should look into it because if this branch never gets called the data.yaml
stuff is confusing.
viper.SetConfigFile("./data.yaml") | ||
case "-": | ||
if isStdInFromTerminal() { | ||
log.Info().Msg("Reading input directly from command line...") | ||
// TODO: this can take up to half a second to output which interrupts the user's experience |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 If this is working correctly users shouldn't see this unless they are manually typing in yaml formatted input. If that's not the case lmk and I can take a closer look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this happens to me only when manually typing in the command. (The check isStdInFromTerminal()
is fine)
What happens is: I start typing, then a second later the message pops up, I lose track of what I've typed, then CTRL+C
and re-run the command and start typing again. It's a UX annoyance
Overall looks good! Just left a few comments 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ticket
#159
Changelog
Tophatting
Create a secret using interactive input:
Create a secret using heredoc:
Create a secret using a file:
Updated a secret using interactive input:
Finally, list secrets to see last updated time: