Fix version check #1157

0pcom · 2022-04-19T03:44:15Z

Fixes #1154

…ions

mrpalide · 2022-04-20T03:08:37Z

It's OK for different patch version, but on minor different get error yet.

cmd/skywire-visor/commands/root.go

jdknives · 2022-04-20T20:01:54Z

cmd/skywire-visor/commands/root.go

@@ -581,3 +446,144 @@ func checkHvIsRunning(addr string, retries int) bool {
 	}
 	return false
 }
+
+func updateConfig(mLog *logging.MasterLogger, conf *visorconfig.V1) {


I am wondering if it would be better to ask the user to run the update CLI command. For most users we will perform config upgrades via package scripts. For others running from source, they could/should be able to run CLI I would think. Would save us the trouble from reimplementing the CLI code here.

I personally never use config update.

Config gen is very solidly implemented, whereas it isn't clear that config update will act as desired and was otherwise untested for updating possibly between versions. Its intended use was only to change the values that cannot be directly set with config gen.

cmd/skywire-visor/commands/root.go

pkg/visor/visorconfig/parse.go

0pcom · 2022-04-20T23:53:17Z

It's OK for different patch version, but on minor different get error yet.

is this not the desired behavior?

The config needs to be updated if there is such a drastic change to the version, I thought?

@jdknives didn't object so I guess my assumption is wrong

I will implement the semver package as you suggest

pkg/visor/visorconfig/parse.go

mrpalide

Based on its issue #1154, no need to check minor update, and if I missing something and need to check then PR is OK.

@ersonp

…ggested by @ersonp

jdknives · 2022-04-26T10:12:23Z

@0pcom correct. Only need to check on major increments. There are a few remaining issues:

[2022-04-26T12:09:19+02:00] INFO [buildinfo]: version="v1.0.0"
[2022-04-26T12:09:19+02:00] INFO [buildinfo]: built on="2022-04-26T10:09:09Z"
[2022-04-26T12:09:19+02:00] INFO [buildinfo]: against commit="c33e3c8fdf0db9443c863ba86b3beafb8da63bc8"
[2022-04-26T12:09:19+02:00] INFO [visor:config]: Reading config from file.
[2022-04-26T12:09:19+02:00] INFO [visor:config]: filepath="skywire-config.json"
[2022-04-26T12:09:19+02:00] INFO [visor:config]: config version: ="v0.6.0-241-g20aa58f7"
[2022-04-26T12:09:19+02:00] ERROR [visor:update-config]: config version does not match visor version
[2022-04-26T12:09:19+02:00] ERROR [visor:update-config]: skywire version: ="v1.0.0"
[2022-04-26T12:09:19+02:00] INFO [visor:update-config]: match:true
[2022-04-26T12:09:19+02:00] INFO [visor:update-config]: updstr:go run cmd/skywire-cli/skywire-cli.go config gen -b
[2022-04-26T12:09:19+02:00] INFO [visor:update-config]: please update your config with the following command:

Logging is too verbose. Most of the info logging should be at debug level. Some of the logs are duplicate such as the last three ones.

jdknives · 2022-04-26T10:16:31Z

cmd/skywire-visor/commands/root.go

+func logBuildInfo(mLog *logging.MasterLogger) {
+	log := mLog.PackageLogger("buildinfo")
+	visorBuildInfo = buildinfo.Get()
+	if visorBuildInfo.Version != "unknown" {


Can we put these logs on one line?

done with 0e6b703

jdknives · 2022-04-26T10:17:34Z

pkg/skyenv/values_darwin.go

@@ -8,6 +8,10 @@ const (
 	OS = "mac"
 	// SkywirePath is the path to the installation folder.
 	SkywirePath = "/Library/Application Support/Skywire"
+	// Ptext is the description to use for the -p flag for config gen and for the visor
+	Ptext = "use mac installation path: " + SkywirePath


I think you misunderstood my request. I would like to get rid of the additional global variable. We can just construct this message inside the flag declaration instead of exposing a global variable.

done with f672c82

jdknives · 2022-04-26T10:18:11Z

pkg/skyenv/values_darwin.go

+	// Ptext is the description to use for the -p flag for config gen and for the visor
+	Ptext = "use mac installation path: " + SkywirePath
+	// Configjson is the config name generated by the script included with the installation on mac
+	Configjson = "skywire.json"


We talked about making this congruent with the names for generic skywire configs by using skywire-config.json. Did that not work out or did you miss it?

I need to implement the systray and config gen at the user level for the packages first

when that is working I will go back and figure out migration of the configs in /opt/skywire from skywire.json to skywire-config.json

It's just too much to do at the same time. I'm trying to avoid unintended consequences.

…h. Move buildinfo logging to one line

0pcom · 2022-04-26T23:51:17Z

@jdknives i've removed the generation of the update command string per our conversation here 0e6b703

jdknives · 2022-04-27T10:19:21Z

cmd/skywire-visor/commands/root.go

@@ -380,7 +378,7 @@ func initConfig(mLog *logging.MasterLogger, confPath string) *visorconfig.V1 { /
 		log.WithError(err).Fatal("Failed to read in config.")
 	}
 	if !compat {
-		updateConfig(mLog, conf)
+		updateConfig(mLog)


This can be one line of just using log.Fatalf() for instance. Seems overkill to me to assign a new package logger and then log error and fatal separately.

you are right, separate func was vestigial - fixed in cd88cfe

0pcom added 2 commits April 17, 2022 14:58

update -p --pkg flag for visor

613cf4c

change fail on incompatible version behavior per @jdknives specificat…

7b2026d

…ions

jdknives reviewed Apr 20, 2022

View reviewed changes

cmd/skywire-visor/commands/root.go Outdated Show resolved Hide resolved

jdknives reviewed Apr 20, 2022

View reviewed changes

cmd/skywire-visor/commands/root.go Outdated Show resolved Hide resolved

jdknives reviewed Apr 20, 2022

View reviewed changes

cmd/skywire-visor/commands/root.go Show resolved Hide resolved

jdknives reviewed Apr 20, 2022

View reviewed changes

cmd/skywire-visor/commands/root.go Outdated Show resolved Hide resolved

jdknives reviewed Apr 20, 2022

View reviewed changes

pkg/visor/visorconfig/parse.go Outdated Show resolved Hide resolved

0pcom added 2 commits April 21, 2022 19:17

remove manual version checks for unknown version

f89711a

make format check

0d73290

ersonp reviewed Apr 25, 2022

View reviewed changes

pkg/visor/visorconfig/parse.go Outdated Show resolved Hide resolved

ersonp reviewed Apr 25, 2022

View reviewed changes

pkg/visor/visorconfig/parse.go Outdated Show resolved Hide resolved

mrpalide reviewed Apr 25, 2022

View reviewed changes

change variable names and remove accidentally included comments as su…

c33e3c8

…ggested by @ersonp

jdknives requested changes Apr 26, 2022

View reviewed changes

0pcom added 3 commits April 26, 2022 18:22

remove generation of update string for visor / config version mismatc…

0e6b703

…h. Move buildinfo logging to one line

fix buildinfo logging order to version-date-commit

d7be6f9

remove global variable skyenv.Ptext

f672c82

jdknives requested changes Apr 27, 2022

View reviewed changes

change logging for incompatible config

cd88cfe

mrpalide merged commit b8998bb into skycoin:develop Apr 28, 2022

0pcom deleted the fix/version-check branch April 29, 2022 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix version check #1157

Fix version check #1157

0pcom commented Apr 19, 2022

mrpalide commented Apr 20, 2022

jdknives Apr 20, 2022

0pcom Apr 21, 2022

0pcom commented Apr 20, 2022

mrpalide left a comment •

edited

Loading

jdknives commented Apr 26, 2022

jdknives Apr 26, 2022

0pcom Apr 26, 2022

jdknives Apr 26, 2022

0pcom Apr 26, 2022

jdknives Apr 26, 2022

0pcom Apr 26, 2022

0pcom commented Apr 26, 2022

jdknives Apr 27, 2022

0pcom Apr 27, 2022 •

edited

Loading

Fix version check #1157

Fix version check #1157

Conversation

0pcom commented Apr 19, 2022

mrpalide commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0pcom commented Apr 20, 2022

mrpalide left a comment • edited Loading

Choose a reason for hiding this comment

jdknives commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0pcom commented Apr 26, 2022

Choose a reason for hiding this comment

0pcom Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

mrpalide left a comment •

edited

Loading

0pcom Apr 27, 2022 •

edited

Loading