go/oasis-node/cmd/config: Add the migrate subcommand #5237

abukosek · 2023-03-22T15:33:29Z

A new migrate subcommand is added to the node's config
command. This subcommand can be used to automatically migrate
the old YAML config file into the new format introduced in
commit 2a132b3.

The subcommand logs the various changes it makes and warns the
user if a config option is no longer supported, etc.
At the end, any unknown sections of the input config file are
printed to the terminal to give the user a chance to review
them and make manual changes if appropriate.

codecov · 2023-03-22T16:42:13Z

Codecov Report

Merging #5237 (2622d56) into master (70b1f66) will decrease coverage by 0.17%.
The diff coverage is 64.03%.

❗ Current head 2622d56 differs from pull request most recent head 19cffd5. Consider uploading reports for the commit 19cffd5 to get more accurate results

@@            Coverage Diff             @@
##           master    #5237      +/-   ##
==========================================
- Coverage   67.23%   67.07%   -0.17%     
==========================================
  Files         512      514       +2     
  Lines       54293    54685     +392     
==========================================
+ Hits        36502    36678     +176     
- Misses      13371    13555     +184     
- Partials     4420     4452      +32

Impacted Files	Coverage Δ
go/oasis-node/cmd/config/migrate/migrate.go	`63.65% <63.65%> (ø)`
go/oasis-node/cmd/config/config.go	`100.00% <100.00%> (ø)`
go/oasis-node/cmd/root.go	`85.36% <100.00%> (+0.36%)`	⬆️

... and 62 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kostko

I would not put this under debug.

Also can you add a cfg changelog entry that describes how to use this command?

abukosek · 2023-03-23T10:30:25Z

Done :)
I've originally put it under debug because the fixgenesis subcommand was also there. We should probably clean that up at some point.

Going to try this out on various config files now and add code to migrate worker.p2p as well (this is the only thing missing, I think).

kostko

Can you also add some quick unit tests for these migrations? E.g. using some dummy configs derived from real ones.

go/oasis-node/cmd/config/migrate/migrate.go

kostko · 2023-04-03T10:49:39Z

go/oasis-node/cmd/config/migrate/migrate.go

+		logger.Warn("note that grpc.* options are from now on only available on the command-line")
+	}
+	if _, ok = oldCfg["metrics"]; ok {
+		logger.Warn("note that metrics.* options are from now on only available on the command-line")


Oh, didn't notice that before, why is this the case, doesn't it make sense to have them in the config file?

I guess we could move them into the config in a separate PR...

go/oasis-node/cmd/config/migrate/migrate.go

peternose

I think that most of the code could be simplified with a map old.config.value => new.config.value and a function which creates all maps on the path, copies values from one config to the other and prints a comment, but not sure.

go/oasis-node/cmd/config/migrate/migrate.go

peternose · 2023-04-03T11:52:59Z

go/oasis-node/cmd/config/migrate/migrate.go

+		logger.Error("output file name missing, use the --out flag to specify it")
+		os.Exit(1)
+	}
+	if cfgInFileName == cfgOutFileName {


This check probably won't work for oasis-node config migrate --in a.yaml --out ./folder/../a.yaml.
Files could also be folders, e.g. oasis-node config migrate --out folder (not sure if we need so thorough validation).

Yeah, this is by design, it's just a safeguard against fatfingering :)

abukosek · 2023-04-03T12:32:34Z

Thank you for your comments!

I also tried to come up with a way to represent and apply the changes in a nicer, more organized way, but the actual changes are too chaotic to do that, so unfortunately this mess of if/else statements is what works best :D

This command will only be useful in the next release. Afterwards, we can remove it or replace its structure with something nicer if necessary.
I don't think overengineering it at this point is worth the time.

peternose · 2023-04-03T12:39:31Z

go/oasis-node/cmd/config/migrate/migrate.go

+	newCfgStruct := config.DefaultConfig()
+	err = yaml.Unmarshal(newCfgRaw, &newCfgStruct)
+	if err != nil {
+		logger.Error("failed to parse config file after migration (this might be normal if you're using environment variable substitutions in your original config file)", "err", err)


This error message is a bit confusing for me, as I expect the problem is in the original file and not in the converted one. I hope the users won't have the same problem.

Options:

Add few extra lines, one printing that we are doing a migration, one that migration was completed, file saved, another that we are validating the migrated file.

Or split migration/validation, e.g. create a new command and print the command for validation at the end. Maybe even add a warning that the file is not validated. New command could be useful latter to verify already migrated config files.

Keep as it is.

Config validation is done by default now when then node starts up and loads the config file, so I don't think a separate command is needed for that.

I will add a few more info logs to let the user know what's happening.

peternose

Looks good.

One last thing. I would maybe change that the mode is always present in the config file, so that you know the node's type as soon as you see the config file, without need to know the default value.

abukosek force-pushed the andrej/feature/config-migration branch 4 times, most recently from 2c035fc to cc4368c Compare March 22, 2023 15:50

kostko reviewed Mar 23, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/config-migration branch from cc4368c to 873ba13 Compare March 23, 2023 10:26

abukosek changed the title ~~go/oasis-node/cmd/debug: Add the migrate-config subcommand~~ go/oasis-node/cmd/config: Add the migrate subcommand Mar 23, 2023

kostko reviewed Mar 28, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/config-migration branch 5 times, most recently from f4dcf82 to 41a1b64 Compare March 30, 2023 12:46

abukosek marked this pull request as ready for review March 30, 2023 13:31

abukosek requested review from peterjgilbert, pro-wh, ptrus and peternose as code owners March 30, 2023 13:31

kostko reviewed Apr 3, 2023

View reviewed changes

go/oasis-node/cmd/config/migrate/migrate.go Outdated Show resolved Hide resolved

abukosek force-pushed the andrej/feature/config-migration branch 2 times, most recently from 52ffc9e to 9a9785c Compare April 3, 2023 11:44

peternose reviewed Apr 3, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/config-migration branch 3 times, most recently from d42cc40 to 17cd76c Compare April 3, 2023 12:28

peternose reviewed Apr 3, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/config-migration branch from 17cd76c to 2622d56 Compare April 3, 2023 14:10

peternose approved these changes Apr 3, 2023

View reviewed changes

go/oasis-node/cmd/config: Add the migrate subcommand

19cffd5

abukosek force-pushed the andrej/feature/config-migration branch from 2622d56 to 19cffd5 Compare April 3, 2023 16:35

abukosek enabled auto-merge April 3, 2023 16:36

abukosek merged commit 8f3cf28 into master Apr 3, 2023

abukosek deleted the andrej/feature/config-migration branch April 3, 2023 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/oasis-node/cmd/config: Add the migrate subcommand #5237

go/oasis-node/cmd/config: Add the migrate subcommand #5237

abukosek commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

kostko left a comment

abukosek commented Mar 23, 2023

kostko left a comment

kostko Apr 3, 2023

abukosek Apr 3, 2023

peternose left a comment

peternose Apr 3, 2023

abukosek Apr 3, 2023

abukosek commented Apr 3, 2023

peternose Apr 3, 2023

abukosek Apr 3, 2023

peternose left a comment

go/oasis-node/cmd/config: Add the migrate subcommand #5237

go/oasis-node/cmd/config: Add the migrate subcommand #5237

Conversation

abukosek commented Mar 22, 2023 • edited Loading

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

kostko left a comment

Choose a reason for hiding this comment

abukosek commented Mar 23, 2023

kostko left a comment

Choose a reason for hiding this comment

kostko Apr 3, 2023

Choose a reason for hiding this comment

abukosek Apr 3, 2023

Choose a reason for hiding this comment

peternose left a comment

Choose a reason for hiding this comment

peternose Apr 3, 2023

Choose a reason for hiding this comment

abukosek Apr 3, 2023

Choose a reason for hiding this comment

abukosek commented Apr 3, 2023

peternose Apr 3, 2023

Choose a reason for hiding this comment

abukosek Apr 3, 2023

Choose a reason for hiding this comment

peternose left a comment

Choose a reason for hiding this comment

abukosek commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading