-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
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.
Did a quick review. Have a look.
flag.StringVar(&address, "a", ":8080", "HTTP server address") | ||
flag.Parse() | ||
|
||
if credential == "" { |
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 would like to see these flags either sourced from:
- The default
~/.secrethub/credential
file - The SECRETHUB_CREDENTIAL envar
- The
--credential
flag
It's not entirely necessary for the MVP, but it would be very nice to make it consistent with current CLI behavior.
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.
Would be nice if the same DRY function could be used there. What do you think @SimonBarendse?
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 agree that would be the preferred end-state.
However, I think for now it’s better to duplicate it in an internal package, so that the cli open-source release doesnt become a blocker for the terrraform open-source release. If we use the same signature, we can replace it (deduplicate) with the cli package when that is open-source.
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.
Few comments, nice changes!
@@ -56,7 +56,7 @@ func startHTTPServer() error { | |||
) | |||
|
|||
fmt.Println("SecretHub Clientd started, press ^C to exit") | |||
return http.ListenAndServe(address, mux) | |||
return http.ListenAndServe(fmt.Sprintf(":%v", port), mux) |
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 %v
works here, but to be 'correct' it should be %d
. If it works, fine.
flag.StringVar(&address, "a", ":8080", "HTTP server address") | ||
flag.StringVar(&credential, "C", "", "(Required) SecretHub credential") | ||
flag.StringVar(&credentialPassphrase, "P", "", "Passphrase to unlock SecretHub credential") | ||
flag.IntVar(&port, "p", 8080, "HTTP port to listen on") |
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.
The SecretHub CLI has a --credential-passphrase
flag with a shortcut -p
. Not sure what we want to do here
Very basic implementation to be able to act as a Terraform state backend