-
Notifications
You must be signed in to change notification settings - Fork 45
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
skywire-cli config priv
subcommand
#1369
Conversation
skywire-cli config priv
subcommand
…r for disabling the hypervisor as a testing consideration.
… subcommand. Add get and set subcommands to priv subcommands in skywire-cli visor and skywire-cli config. move Privacy struct to skyenv.
pkg/visor/api.go
Outdated
*/ | ||
|
||
// SetPrivacy implements API. | ||
func (v *Visor) SetPrivacy(p skyenv.Privacy) error { |
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.
Not sure if its a wise move to make the visor binary dependent on the existence of the skywire-cli
command being existent and in $PATH. Why not write to the config yourself 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.
Also, it appears to me like we are calling SetPrivacy
function inside the skywire-cli config priv set
call via RPC. And then inside this function we are calling the cli function again. Are we sure this works.
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 is my strong conviction that the visor should be writing its own configs. The cli is tasked with config generation and updating.
the issue we have had previously is with having multiple implementations of the same thing, and a certain bifurcation or lack of feature parity existed in that.
with this, the config is only written by the cli, and everything else calls upon that existing functionality.
the visor is, in a practical sense, dependant on skywire-cli. We don't explicitly support omitting skywire-cli from the installation. I don't know how you could practically or reliably run skywire without updating the config on version changes, that sort of thing. You would need skywire-cli at some point and there is no reason to separate the binaries.
lets please assume it's in the path and not go down such rabbit holes. If the cli cannot be assumed to be present with the visor i will begin to question my sanity.
it's not like it breaks the visor if there is a command not found error there.
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 way the code is written right now is overly complicated and the API part of it won't even work because it simply calls itself. I think there is a way to fix it by simply moving the code that writes to the privacy.json
to internal or to it's own pkg and that way both the instances will be using the exact same code.
Also, there is no need to use the cli in this way since we are in the same codebase and can access the code programmatically. Because of the drawbacks of not having cli in the PATH using the current way may not cause any issues when installed by packages but will most certainly cause unnecessary headaches and issues while development and testing and also on a manual install/build.
The "default" (for testing) is the genesis address
demonstrating using
demonstrating setting a custom path and setting display_node_ip true
the user will either set the privacy.json file via the underlying command executed is config generation is intentionally done exclusively via skywire-cli, as it is not appropriate for the visor to generate or update configs except for very limited circumstances such as updating app autostart via the hypervisor UI
This seems like it should create the config with the wrong permissions for the path but in fact it does not
the config was removed here without permissions errors
|
Did you run
make format && make check
?yes
Fixes
#1361
#1362
Adds:
skywire-cli config priv
subcommands toset
andget
rewards address and privacy with privacy.jsonskywire-cli visor priv
subcommands for setting privacy over rpcHow to test this PR: