Skip to content
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

Group skywire-cli visor subcommands #1132

Merged
merged 68 commits into from
Apr 6, 2022
Merged

Group skywire-cli visor subcommands #1132

merged 68 commits into from
Apr 6, 2022

Conversation

0pcom
Copy link
Collaborator

@0pcom 0pcom commented Mar 11, 2022

This PR changes the skywire-cli interface by adding another layer of sub-commands to skywire-cli visor

This may affect any scripts which use these sub-commands

An effort was made to shorten some of the more lengthy descriptions, and to add shorthand flags wherever possible, especially for all of the existing skywire-cli config gen flags. The changes are extensive but not comprehensive.

Did you run make format && make check?
yes
Fixes #1072 #1138 #1134

Changes:
previous:
image

now:
image

How to test this PR:
go run cmd/skywire-cli/skywire-cli.go visor --help

I will update the documentation in a subsequent PR after this one is merged.

STDOUT Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor

ersonp commented Mar 24, 2022

The setup node PK stays the same 026c2a3e92d6253c5abd71a42628db6fca9dd9aa037ab6f4e3a31108558dfd87cf even if the -t flag is not specified. The setup node pk for prod is 0324579f003e6b4048bae2def4365e634d8e0e3054a20fc7af49daf2a179658557.

This is related to

This overwrites the Url to the dev ones from local no matter what is Services are passed.

and

We need an if statement here for the -t flag. If it is set we need to call /config/dev to get dev urls.

@ersonp
Copy link
Contributor

ersonp commented Mar 24, 2022

I think either we should move the logic of serviceConf and fallback to the package visorconfig and allow visor to rewrite the config in case the config is old. Or keep the serviceConf and fallback in CLI, but not allow the visor to run at all if the config version is old, prompting the user to update the config via the CLI.

@ersonp
Copy link
Contributor

ersonp commented Apr 4, 2022

If we are moving towards the version of the config and visor being the same, we need to add a check during visor startup to see if the version of the config matches the version of the visor; if not then we need to stop the startup and print the required steps to update the config via the CLI.

@ersonp
Copy link
Contributor

ersonp commented Apr 4, 2022

erson@erson-69:~/Workspace/Go/skywire$ ./skywire-visor -c skywire-config.json 
specified config file: skywire-config.json
found config file: skywire-config.json
Version "v0.6.0-139-g09109d5d" built on "2022-04-04T12:17:15Z" against commit "09109d5dabed7ba5e7721265e60c2a13df9801fa"
[2022-04-04T17:54:34+05:30] INFO [visor:config]: Reading config from file. filepath="skywire-config.json"
v0.6.0-139-g09109d5
dmsg servers detected
tracer
[2022-04-04T17:54:34+05:30] INFO [visor:startup]: Begin startup. public_key=038a33fc20b568b3e40f0bba1ab4ebd9eba938e33eb1d0904ccad1320911be7643
[2022-04-04T17:54:34+05:30] INFO [visor]: Starting
[2022-04-04T17:54:34+05:30] INFO [stcpr]: Starting

From the startup logs

specified config file: skywire-config.json
found config file: skywire-config.json
v0.6.0-139-g09109d5
dmsg servers detected
tracer

printing this seems to be unnecessary as these logs provide pretty much the same info

Version "v0.6.0-139-g09109d5d" built on "2022-04-04T12:17:15Z" against commit "09109d5dabed7ba5e7721265e60c2a13df9801fa"
[2022-04-04T17:54:34+05:30] INFO [visor:config]: Reading config from file. filepath="skywire-config.json"

@ersonp
Copy link
Contributor

ersonp commented Apr 4, 2022

We also need to update the changelog.

@0pcom
Copy link
Collaborator Author

0pcom commented Apr 5, 2022

@ersonp

image

@jdknives jdknives merged commit 4aa8295 into skycoin:develop Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants