-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
R4R: Remove unimplemented gaiacli init command #3156
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3156 +/- ##
==========================================
- Coverage 55.05% 55% -0.06%
==========================================
Files 133 132 -1
Lines 9438 9425 -13
==========================================
- Hits 5196 5184 -12
Misses 3922 3922
+ Partials 320 319 -1 |
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.
Minor remark, but otherwise LGTM.
client/rpc/root.go
Outdated
// cli version REST handler endpoint | ||
func CLIVersionRequestHandler(w http.ResponseWriter, r *http.Request) { | ||
v := version.GetVersion() | ||
w.Write([]byte(v)) |
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 we should return a simple JSON response here.
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.
Yes we should. Like that idea.
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 👍
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.
It's true that this command doesn't do anything at the moment, but I think it is intended to - definitely, we want some way for lite clients to choose a root-of-trust explicitly for verification. Maybe this isn't the way, but I wonder if we just haven't done the implementation yet.
cc @ebuchman do you have more context here?
@cwgoes I don't think that feature is anywhere close to done so we can easily add it back. This has been dead code confusing users and cluttering up the CLI for a while. |
OK - I split my concerns into an issue, I do think this merits discussion but we can remove the command for now. |
Title
docs/
)PENDING.md
with issue #Files changed
in the github PR explorer