package defaults with gen-config -p #579
Conversation
|
|
There was a problem hiding this comment.
@0pcom thanks for this PR. Generally this is a good idea and we will go with it. We will use the -p flag to generate configs for package installations. Two points:
-
We will only release official packages with the next release. Therefore, this should target
developinstead ofmaster. The config will change on that release (you can check the changes by testing develop) a bit. We got rid of thehypervisorbinary and instead a visor can now expose the UI if config was generated with--hypervisorflag. That necessitates some changes to this PR. -
We have discussed with Synth to put package based installations to
$HOME/skywirelong term as it does not require root access. Change installation directory #581
In anticipation of that change, you can probably already change your PR here.
| @@ -17,21 +17,24 @@ import ( | |||
| var ( | |||
| output string | |||
| replace bool | |||
| package1 bool | |||
There was a problem hiding this comment.
Could you please rename it to something like packageConfig?
| @@ -68,6 +71,9 @@ var genConfigCmd = &cobra.Command{ | |||
| log.Fatalln("Error retaining old keys", err) | |||
| } | |||
| } | |||
| if package1 { | |||
There was a problem hiding this comment.
What do you think of moving this condition before the switch statement and having switch inside an else?
| sk cipher.SecKey | ||
| output string | ||
| replace bool | ||
| package1 bool |
There was a problem hiding this comment.
Could you please rename it to something like packageConfig?
I suppose I should retarget this PR at develop but I am unfamiliar with how to do that at this stage without filing a new PR. Additionally it would appear that this would require some revision to work with the new setup that you have described. I will take your advice on where to go from here. On the installation path, it would be OK to use the $HOME/skywire path except that by default the service is running as root. These paths should be created at /root/skywire by default when the config generation is preformed as root. Is it possible at this stage to run skywire as an unprivlaged user? The last time I tried to I recall there was some issue. @nkryuchkov i will be able to preform the requested changes tonight. Per the switch / else; I have no opposition but I will have to figure out how to implement that. |
|
@0pcom I just redirected the PR at |
|
re-pening from a different branch |
Did you run
make format && make check?yes
Fixes #
#572
Changes:
How to test this PR:
recompile the binaries
Notes:
I did not make extensive efforts to integrate it with the existing logic of the other flags, nor did I test combinations of these flags. It solves my immediate issue and can be improved upon in the future.
I request that this PR be revewed / approved as soon as possible as I wish to update to the VPN-containing version but as stated in #564 when I tested this on my machine, the visor failed to start after the update because of issues with the config. I have not tested or attempted to reproduce #564 on arm64, and no other user has complained of this to my knowledge, so perhaps it is some anomaly to do with the architecture or specifically that binary. I don't know.
However, I can say that the content of this pull request is critical for reliably automating configuration at the point of installation, eliminating the need for manual revision of the config file.