source a .conf file with skywire-cli config gen
#1558
Conversation
… tp setup node configuration
0pcom
changed the title
source a .conf file with skywire-cli config gen [WIP]source a .conf file with skywire-cli config gen
|
|
pkg/visor/visorconfig/values.go
Outdated
| func MustPKs(pks string) []cipher.PubKey { | ||
| var sPKs cipher.PubKeys | ||
| if err := sPKs.Set(pks); err != nil { | ||
| fmt.Printf("invalid public key or keys: %s", pks) |
There was a problem hiding this comment.
I see that this package uses fmt consistently. Is there a reason we are using fmt here instead of the logging pacakge we use typically?
There was a problem hiding this comment.
There is no logging package already used in this particular file, but fmt was already imported
i was replacing a panic with an actual error before the panic because at one point i was hitting that panic and it was very unhelpful.
There was a problem hiding this comment.
@0pcom we use the logging package liberally throughout the project. I dont see a reason we should avoid importing it here unless you see an explicit reason.
pkg/visor/visorconfig/values.go
Outdated
| panic(err) | ||
| } | ||
|
|
||
| return sPK | ||
| } | ||
|
|
||
| // MustPKs unmarshals comma separated list of string PKs to []cipher.PubKey. It panics if unmarshaling fails. | ||
| func MustPKs(pks string) []cipher.PubKey { |
There was a problem hiding this comment.
Can we name this more explicitly like MustParsePKs?
There was a problem hiding this comment.
If we want to rename the one above it to match as MustParsePK
| if err != nil { | ||
| log.WithError(err).Fatal("Failed to create http request\n") | ||
| } | ||
| req.Header.Add("Cache-Control", "no-cache") |
There was a problem hiding this comment.
Where is the cache supposed to come from? I dont think we are currently caching the conf service responses.
There was a problem hiding this comment.
I cannot say, this was existing code I just moved
| gHiddenFlags = append(gHiddenFlags, "binpath") | ||
| // genConfigCmd.Flags().StringVar(&addSkysocksClientSrv, "proxyclientpk", scriptExecString("${PROXYCLIENTPK}"), "set server public key for proxy client") |
| if err != nil { | ||
| //silence errors for stdout | ||
| if !isStdout { | ||
| log.WithError(err).Error("Failed to fetch servers\n") | ||
| log.Warn("Falling back on hardcoded servers") | ||
| } | ||
| } else { | ||
| // nil error from client.Do(req) | ||
| // nil error from client.Get |
There was a problem hiding this comment.
Can we move all of this code to inside the else clause outside the else clause? Resp body should be closed even if there is an error I believe. Also, can we set the log level to panic or something equivalent and remove all these checks for !stdout?
There was a problem hiding this comment.
valid points, will make the changes
There was a problem hiding this comment.
I've outdented the code as requested, but in terms of getting rid of 'all these checks for !isStdout' it's not just as straightforward as changing the log level because the behavior is also changed by the stdout flag to not create or read files and I already have the -w flag as an override for showing the errors, so it's something I would like to revisit after this PR is merged.
We are talking about code that currently works and is on develop in any case
|
I've gone ahead and implemented the dead code which was functionality from config update also updated the env file |
skywire-cli config genhas a lot of flags, but still lacks the granularity of configuration which may be desired to persist or set initially for config generation.This PR introduces a comment-able, human-editable standard .conf template which can be specified as an environmental variable to
config genandsourced(or the windows / powershell equivalent)Here is the WIP version for linux
This will be extended to allow for or include
Invocation
to generate a json skywire config using such a template, the path to the .conf file should be specified as an env to
skywire-cli config genas followsThe values override the defaults of the flags, viewable with
config gen --allas demonstrated in the following screenshots:no env file specified or file not detected
Here I have
Usage
The use of the env file and flags will be to a large extent mutually exclusive with flags, as using the same flagset (for boolean values) as envs specified in the .config will cancel each other out. Some flags will not be made available to set in the env file; specifically the following flags
-nstdout-xretain hypervisors-rregenerate config or retain keysadditionally, more flags need to be added to achieve the desired level of granularity of configuration.
Non-linux implementations
(.)command on windows (powershell) is the equivalent of thesourcecommand on linux.$in front of variable declarations