Config: Add versioning #1671

gbjk · 2024-10-10T01:05:04Z

Config

Add config versioning manager. Motivation:
- Huobi would like to move futures to coinmargined futures, but we don't want to lose the user's existing config
- We have a number of places we have deprecated config clutter, and pointers to native types, purely so we can upgrade it.
Version1 handles the pairs manager format migration
Version2 handles GDAX to Coinbase rename
Version3 will handle AssetEnabled, but keeping this initial PR minimal
Upgrade configtest to Version2
Not upgrading config_example because it would jump over some null changes - I'll handle those in Version4 quickly
Add -decrypt and -upgrade to cmd/config, no longer explicitly decrypts, or automatically reverses what was there
Add -edit to edit a file in place
-in instead of -infile and -out instead of outfile
Do not echo input to user when entering encryption key
🐛 Fix encryption keys not allowing spaces
This has some light touch improvements to encryption handling. I think I'm going to have to circle back to that separately later.

Note Well: The test failure in TestConfigAllJsonResponse is fixed by the dependency below

Dependencies

currency: fix edge case on NewCode where all string characters are digits #1593 fixes TestConfigAllJsonResponse

Type of change

New feature (non-breaking change which adds functionality)

shazbert

Good stuff. Will give it a thorough testing next round. Thanks.

cmd/config/config.go

config/config_test.go

config/config.go

config/versions/versions.go

gbjk · 2024-10-13T05:45:44Z

@shazbert I'm done restructuring this.
All versions live in the same package, in separate files.
Stateful representations of types used for state-in-time upgrades live in vN/types.go
Not expecting types for each version. A type that changes a thing can introduce the new representation, and if necessary add the old representation to a previous version as well.

shazbert

Nice changes! Thanks for changing the requested. Also just mentioning config_example.json can we now shift that up in line with this versioning system, I see you have commited the configtest.json file.

cmd/config/config.go

config/versions/versions.go

cmd/config/config.go

config/config.go

config/config_test.go

gbjk · 2024-10-17T02:27:15Z

Nice changes! Thanks for changing the requested. Also just mentioning config_example.json can we now shift that up in line with this versioning system, I see you have commited the configtest.json file.

See PR description 🙂

Not upgrading config_example because it would jump over some null changes - I'll handle those in Version4 quickly

The test config I could upgrade without introducing any new nulls. I know I could fix those, or run a whole instance on it to get them fixed, but I want to use it as a test case for making sure our fully versioned upgrade path handles all the *bool examples

shazbert

Thanks sir, just a diff with changes that needs to be added then looks good to go after merge with master. Nice work!

config/config_encryption.go

config/config_test.go

shazbert

Thanks. Nothing else from me.

gbjk · 2024-11-10T10:08:01Z

Rebased.

I've dropped out the "upgrade test config" change. I've realised that I should work through moving all of the things that trigger warnings to proper versions first, then it won't even matter. That's pretty trivial but I'll let this land as-is first.
Otherwise we'll get endless conflicts until this is done.

The method to upgrade it is to run config tool with upgrade, then remove anything that would be nulled out.
Which is precisely why I'm not doing it now.

codecov · 2024-11-10T10:11:36Z

Codecov Report

Attention: Patch coverage is 68.71166% with 153 lines in your changes missing coverage. Please review.

Project coverage is 37.12%. Comparing base (d4c4bf1) to head (d4eb810).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/config/config.go	0.00%	93 Missing ⚠️
config/config.go	82.91%	22 Missing and 12 partials ⚠️
config/versions/versions.go	88.18%	10 Missing and 3 partials ⚠️
config/versions/v1.go	72.72%	6 Missing and 3 partials ⚠️
config/config_encryption.go	91.17%	2 Missing and 1 partial ⚠️
config/versions/v2.go	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1671      +/-   ##
==========================================
+ Coverage   37.04%   37.12%   +0.07%     
==========================================
  Files         414      418       +4     
  Lines      180305   180303       -2     
==========================================
+ Hits        66800    66941     +141     
+ Misses     105657   105497     -160     
- Partials     7848     7865      +17

Files with missing lines	Coverage Δ
common/common.go	`93.35% <ø> (ø)`
config/versions/v0.go	`100.00% <100.00%> (ø)`
engine/engine.go	`48.89% <100.00%> (ø)`
config/versions/v2.go	`92.30% <92.30%> (ø)`
config/config_encryption.go	`85.93% <91.17%> (+3.58%)`	⬆️
config/versions/v1.go	`72.72% <72.72%> (ø)`
config/versions/versions.go	`88.18% <88.18%> (ø)`
config/config.go	`87.09% <82.91%> (+0.05%)`	⬆️
cmd/config/config.go	`0.00% <0.00%> (-8.63%)`	⬇️

... and 26 files with indirect coverage changes

gloriousCode

I'm excited for config versioning. Just did some light prodding

cmd/config/config.go

engine/engine.go

config/versions/v0.go

gloriousCode

Thank you for the latest updates. One main thing I encountered before continuing

config/versions/versions.go

cmd/config/config.go

shazbert

One nit others are commentary. Nice work!

config/config_encryption.go

cmd/config/config.go

config/config.go

config/config_encryption.go

shazbert

Ch ACK! Thank you 🖖

thrasher-

Tested everything and looks to all be good aside from a basic issue. Great work on this feature!

cmd/config/config.go

thrasher-

Thanks for making those changes! Just some remaining minor consistency issues and then this looks good to go!

config/versions/versions_test.go

This restructure allows us to share types between versions, avoids needing to import the versions, and puts the test fixtures in same package. It's a win on all fronts

Called from engine before logger is inited, and also just wrong to use log to communicate with user

Checking the versions at Deploy is much saner.

This simplifies the handling for encryption prompts by moving it to a field on config, allowing us to simplify all the places were were passing around config Also moves password entry to being secure (echo-off)

gbjk · 2024-12-09T03:33:37Z

Rebased and squashed following last fixes, which were cosmetic.

thrasher-

tACK, nice work 🚀

gbjk force-pushed the feature/config_version branch from 70797f0 to 3d955f8 Compare October 10, 2024 01:06

gbjk self-assigned this Oct 10, 2024

gbjk requested review from shazbert, gloriousCode and thrasher- October 10, 2024 01:40

gbjk added the review me This pull request is ready for review label Oct 10, 2024

gbjk mentioned this pull request Oct 10, 2024

Config: Version2 - AssetEnabled #1673

Closed

4 tasks

shazbert requested changes Oct 11, 2024

View reviewed changes

gbjk force-pushed the feature/config_version branch 2 times, most recently from 522cb73 to e8546a6 Compare October 13, 2024 05:34

shazbert requested changes Oct 17, 2024

View reviewed changes

cmd/config/config.go Outdated Show resolved Hide resolved

config/versions/versions.go Outdated Show resolved Hide resolved

cmd/config/config.go Show resolved Hide resolved

config/config.go Show resolved Hide resolved

config/config_test.go Show resolved Hide resolved

gbjk requested a review from shazbert October 19, 2024 10:00

shazbert requested changes Oct 28, 2024

View reviewed changes

config/config_encryption.go Show resolved Hide resolved

config/config_test.go Show resolved Hide resolved

gbjk requested a review from shazbert October 28, 2024 02:39

shazbert approved these changes Oct 28, 2024

View reviewed changes

shazbert added the rebase/merge of master required label Oct 28, 2024

gbjk mentioned this pull request Nov 10, 2024

Overhaul futures asset config storage and usage within GCT #1691

Open

gbjk force-pushed the feature/config_version branch 2 times, most recently from a35693a to 02b027d Compare November 10, 2024 10:06

gbjk removed the rebase/merge of master required label Nov 10, 2024

gbjk force-pushed the feature/config_version branch from 02b027d to fff95a1 Compare November 11, 2024 01:14

gloriousCode requested changes Nov 13, 2024

View reviewed changes

cmd/config/config.go Outdated Show resolved Hide resolved

cmd/config/config.go Outdated Show resolved Hide resolved

engine/engine.go Outdated Show resolved Hide resolved

config/versions/v0.go Show resolved Hide resolved

gbjk requested a review from gloriousCode November 13, 2024 05:12

gbjk force-pushed the feature/config_version branch 2 times, most recently from 23abfa0 to 1b498f3 Compare November 18, 2024 07:40

gloriousCode requested changes Nov 20, 2024

View reviewed changes

config/versions/versions.go Show resolved Hide resolved

config/versions/versions.go Outdated Show resolved Hide resolved

config/versions/versions.go Outdated Show resolved Hide resolved

config/versions/versions.go Show resolved Hide resolved

cmd/config/config.go Show resolved Hide resolved

gbjk requested a review from shazbert November 26, 2024 03:49

gbjk mentioned this pull request Nov 29, 2024

Config: AssetEnabled upgrade #1735

Open

3 tasks

gloriousCode added the gcrc GloriousCode Review Complete label Dec 1, 2024

shazbert requested changes Dec 2, 2024

View reviewed changes

config/config_encryption.go Outdated Show resolved Hide resolved

cmd/config/config.go Show resolved Hide resolved

config/config.go Show resolved Hide resolved

config/config_encryption.go Show resolved Hide resolved

gbjk force-pushed the feature/config_version branch from dbb9635 to 8181ec3 Compare December 2, 2024 03:51

gbjk requested a review from shazbert December 2, 2024 03:51

shazbert approved these changes Dec 2, 2024

View reviewed changes

gbjk mentioned this pull request Dec 4, 2024

Config: OrderManager upgrade #1737

Open

3 tasks

shazbert added the szrc shazbert review complete label Dec 5, 2024

thrasher- requested changes Dec 6, 2024

View reviewed changes

cmd/config/config.go Show resolved Hide resolved

thrasher- reviewed Dec 6, 2024

View reviewed changes

cmd/config/config.go Outdated Show resolved Hide resolved

gbjk requested a review from thrasher- December 7, 2024 04:01

gbjk force-pushed the feature/config_version branch from c568878 to 82e885c Compare December 7, 2024 08:16

thrasher- requested changes Dec 9, 2024

View reviewed changes

gbjk added 11 commits December 9, 2024 10:31

Config: Version Management

0999c94

Engine: Improve visibility of TestConfigAllJsonResponse failures

6b999f2

Config: Update cmd/config to allow upgrades

6fcb5fa

Config: Add Version2 to rename GDAX

f896303

Config: Restructure versioning to share types

a51bd33

This restructure allows us to share types between versions, avoids needing to import the versions, and puts the test fixtures in same package. It's a win on all fronts

Config: Fix SetNTPCheck using log

f3a5a94

Called from engine before logger is inited, and also just wrong to use log to communicate with user

Config: Improve TestMigrateConfig

1103812

Config: Drop requirement for versions to be registered in sequence

b2f0d2d

Checking the versions at Deploy is much saner.

Config: Fix file encrypted but flag not set

4c8b840

Config: Add -edit and encryption upgrade to cmd/config

36e766a

This simplifies the handling for encryption prompts by moving it to a field on config, allowing us to simplify all the places were were passing around config Also moves password entry to being secure (echo-off)

Tests: Fix inconsistent should/must assertions

262c5d3

gbjk force-pushed the feature/config_version branch from 6709cf7 to 262c5d3 Compare December 9, 2024 03:31

thrasher- approved these changes Dec 9, 2024

View reviewed changes

thrasher- merged commit 219ed90 into thrasher-corp:master Dec 9, 2024
5 of 11 checks passed

gbjk deleted the feature/config_version branch December 9, 2024 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config: Add versioning #1671

Config: Add versioning #1671

gbjk commented Oct 10, 2024 •

edited

Loading

shazbert left a comment

gbjk commented Oct 13, 2024

shazbert left a comment

gbjk commented Oct 17, 2024 •

edited

Loading

shazbert left a comment

shazbert left a comment

gbjk commented Nov 10, 2024

codecov bot commented Nov 10, 2024 •

edited

Loading

gloriousCode left a comment

gloriousCode left a comment

shazbert left a comment

shazbert left a comment

thrasher- left a comment

thrasher- left a comment

gbjk commented Dec 9, 2024

thrasher- left a comment

Config: Add versioning #1671

Config: Add versioning #1671

Conversation

gbjk commented Oct 10, 2024 • edited Loading

Config

Dependencies

Type of change

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Oct 13, 2024

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Oct 17, 2024 • edited Loading

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Nov 10, 2024

codecov bot commented Nov 10, 2024 • edited Loading

Codecov Report

gloriousCode left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

gbjk commented Dec 9, 2024

thrasher- left a comment

Choose a reason for hiding this comment

gbjk commented Oct 10, 2024 •

edited

Loading

gbjk commented Oct 17, 2024 •

edited

Loading

codecov bot commented Nov 10, 2024 •

edited

Loading