-
Notifications
You must be signed in to change notification settings - Fork 21
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
Introduce vspadmin binary. #476
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.
I reviewed the code and looks fine.
That said, I personally absolutely detest running things that require separate applications like an admin binary to configure them before they'll even work. Software should just work by default and then you can tweak it from there if you want. It is incredibly annoying to run something for the first time only to have it to tell you that you need to go run something else first when it could just do the setup itself.
I'm not the maintainer of this repo, so it's not my decision and that's fine, but I disagree with this change so much that I won't personally be approving it. Others can feel free to approve it if they're on board with it.
cmd/vspadmin/README.md
Outdated
Example: | ||
|
||
```no-highlight | ||
go run ./cmd/vspadmin createdatabase <xpub> |
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 personally think go install
with a path to the repo is much preferable over run like this. First, you have to clone the repo to be able to do the go run
like this. Second, you have to build the bin every time with go run
.
On the other hand, go install github.com/decred/vspd/cmd/vspadmin@latest
works without cloning and installs the binary.
The same applies to the other cases too, but I'll just comment on this one.
I really appreciate the review even if you don't agree with the premise, thanks.
While I strongly agree with this sentiment, I don't believe it's possible with vspd because there are several things which must be manually configured before vspd is able to operate - e.g. an xpub key for collecting fees, connection details for voting wallets. Do you have any other suggestions for how to solve this problem? I guess it would be possible to add default values which get the process to start without any manual config, but then it would just throw a ton of errors and not actually be able to operate. I'll also mention that this PR is not making the existing situation worse, perhaps just shining a light on it. The manual steps were already required and they are still required after - the only thing which has changed is which binary needs to be executed. |
createdatabase - can vspd make the database if it doesn't exist? writeconfig - have vspd write one if it doesn't exist, and then exit. |
@dajohi - That is precisely how it worked before this PR, the functionality to perform the two tasks has just moved from one bin to another. |
I'm aware there are several things that have to be configured, such as the fee xpub. The same type of scenario is true (except for seeds instead of a pubkey) in e.g. dcrwallet, dcrdex, Cryptopower, etc. Requiring the user to perform some setup, preferably guided by the application itself, is something that is totally expected and often can't be avoided, such as in the case of the xpub. However, there is a very big difference between them needing to set a config flag, add an entry to a config file, etc, and needing to acquire and run an entirely separate binary. I know it might not seem like much for we dev types, but having to go compile an entirely separate binary (and with the instructions as presented here, you even need to first go download, install, and configure golang itself to be able to do the To me, it is a much better user experience to simply complain with a detailed message if things are required. For example, if you were to try to run vspd and it said "An extended public key is required to be set via the --feexpub option .... etc`. It's pretty clear what needs to be done and there is no whole other set of hoops to jump through to go find and build another binary, configure that other binary to point at the right config locations, etc, etc. Since vspd already offers a web-based interface, an even better approach, imo, would be to do it it dcrdex does. When you run it, it points the user at the web address and provides a graphical setup wizard to do all of the one-time setup things. It's certainly a little bit more work, but it provides a much nicer UX overall. If you're concerned about being able to automate it, that could still work via providing the right combination of CLI options. |
Perhaps the database creation and config writing could stay in the vspd binary, but the original inspiration behind the new binary was implementing xpub changing. I considered adding it to the web UI but I think it is too sensitive of an action to be exposed on the internet, especially when it could be implemented on the server CLI with less effort. Shoe-horning it into the existing CLI flags feels really clumsy and not intuitive. Its interesting that you mention dcrdex as your example because actually that is where the inspiration for this PR came from. The dcrdex client does indeed guide the user through an easy GUI, which is obviously correct because it is an application aimed at end users. The server however is composed of a number of CLI bins, notably the main server daemon and a separate admin binary (as well as some other small utilities). Its a more elegant solution than just cramming everything into one binary IMO. We have specified in the VSP operator guide ever since dcrstakepool launched that operators are expected to be experienced sysadmins and required to build go from source, so I'm not sure I agree that running a different bin is a heavy burden. To be brutally honest about it, if somebody cant figure out how to run a binary or how to build go from source, we probably don't want them operating a VSP anyway. |
vspadmin is a new binary which implements one-off admin tasks which are necessarily interactive and thus do not fit neatly into the long-lived vspd binary. The first behaviour added to vspadmin is the ability to create a new vspd database. This behaviour is removed from vspd with the associated config option deprecated. Documentation and scripts updated to reflect the change.
A new command in vspadmin writes the default config file for a new vspd deployment. The behaviour is removed from vspd and documentation has been updated to reflect the change.
Understood. I mean, you're the repo lead, so it's up to you. It's just a serious pet peeve of mine having separate bins for basic setup tasks. Interesting point about DEX, though I'll note that I've never had to run any different bins for DEX despite running it for years at this point. |
vspadmin is a new binary which implements one-off admin tasks which are necessarily interactive and thus do not fit neatly into the long-lived vspd binary.
The initial features added to vspadmin are creating blank databases and default config files, both part of the deployment procedure for a new instance of vspd.
In future this binary can become home for additional features such as changing the admin password (#281, #311) and changing the fee xpub (#461).
Closes #465