iss#6: implemented logic to determine how skywire-updater finds config file. #7
Conversation
…following order 1) CLI argument, 2) ENV, 3) List of default path - i) working directory, home (.skycoin/skywire/skywire-config.json), iii) local (/usr/local/skycoin/skywire/skywire-config.json). Code base is based on library from Skywire mainnet's internal/pathutil/configpath.go.
…ult config path is used. Coverage for test is low.
…en using skywire-update [config-path]. It turns out that adding cobra subcommands prevents RootCmd from accepting path arguments. Therefore, 'updateCmd' was created so update services can take an optional argument. Update will be based on config path loaded from the following order: 1) CLI argument 2) env 3) list of default path.
"merged develop_v2 to develop_v1. issue#6 - Refractor skywire-update. 'Invalid command' was returned when using skywire-update [config-path]. It turns out that adding cobra subcommands prevents RootCmd from accepting path arguments. Therefore, 'updateCmd' was created so update services can take an optional argument. Update will be based on config path loaded from the following order: 1) CLI argument 2) env 3) list of default path. "
There was a problem hiding this comment.
Great work on a first PR! Code is very clean and excellent work in general.
However, it would be appreciated if you implement the changes as below.
Additionally, if you can do a PR in skycoin/skywire to move /internal/pathutil to /pkg/util/pathutil, and update this PR accordingly, that will be a plus.
Please contact me on Telegram @evanlinjin regardless.
README.md
Outdated
|
|
||
| Usage: | ||
| skywire-updater [config-path] [flags] |
There was a problem hiding this comment.
I would prefer it if we read the [config-file] via a CLI flag, instead of a new sub-command. 😜
There was a problem hiding this comment.
Thank you! I was thinking to use CLI flags originally too; I will go back and review the code again and see if I could make this change. Thank you!
There was a problem hiding this comment.
Hi Evan. I made a change to the code. It can now reads in [config-file] via CLI flag instead of having to type a new Cobra subcommand. It turns out that by default Cobra only parses local flags on the target command, any local flags on parent commands are ignored. So by enabling this argument Command.TraverseChildren allows Cobra to parse local flags on each command before executing the target command. Please let me know if this logic is okay. Thanks!
|
@atang152 in the future, I would name the branch |
1) Refractor code to now read in [config-file] via CLI flag instead of using a new Cobra subcommand (As per skycoin#7 (comment)). Previous issue of not being able to accept flag arguments due to addition of subcommand was resolved by enabling Command.TraverseChildren. Enabling TraverseChildren allows Cobra to parse local flags on each command before executing target subcommands. 2) Moved /internal/pathutil to /pkg/util/pathutil
@atang152 How is progress with this? Just referencing this here as I see the source comment is outdated. |
1) Fixed import path by importing github.com/skycoin/skywire/pkg/util/pathutil, rather than copying the source to github.com/skycoin/skywire-updater/... Done so by using GO111MODULE=on go get github.com/skycoin/skywire/pkg/util/pathutil@mainnet 2) Refractor code to now read in [config-file] via CLI flag instead of using a new Cobra subcommand (As per skycoin#7 (comment)). Previous issue of not being able to accept flag arguments due to addition of subcommand was resolved by enabling Command.TraverseChildren. Enabling TraverseChildren allows Cobra to parse local flags on each command before executing target subcommands.
1) Removed pkg/util/pathutil from src and import it directly from skycoin/skywire instead. 2) Ran GO111MODULE=on go mod vendor to update vendor directory to include all packages needed to build/test based on go.mod and go src code.
Ran goimports with the correct path. Editted Makefile to match Skywire standardize format.
Implement services to find config file path based on the following order:
Code is based on library from Skywire mainnet's internal/pathutil/configpath.go. 'Invalid command' was returned when using the original skywire-update [config-path] code. It turns out that adding subcommands prevents RootCmd from accepting path arguments. Therefore, 'updateCmd' was created so update services can take in an optional path argument.
First contribution. Not sure if it's done correctly but I hope it helps. Thank you.