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

Fix loadNodeConfig loses DB data #5597

Open
friofry opened this issue Jul 26, 2024 · 1 comment
Open

Fix loadNodeConfig loses DB data #5597

friofry opened this issue Jul 26, 2024 · 1 comment
Assignees
Labels
bug core-team NodeConfig Anything related to NodeConfig
Milestone

Comments

@friofry
Copy link
Contributor

friofry commented Jul 26, 2024

Problem

As mentioned in status-im/status-desktop#14929
OverwriteNodeConfigValues loses some data (LogLevel, Waku nodes).
Here is an initial fix: #5527

Since loadNodeConfig is used in different ways in different places, this ticket should be done after #5563 (make paths in NodeConfig consistent) because conf.DataDir = filepath.Join(b.rootDataDir, conf.DataDir) joins 2 absolute paths in some tests.

Another issue: we load the DataDir from the db in conf, err := nodecfg.GetNodeConfigFromDB(b.appDB), then
merge it with the default config conf, err = b.OverwriteNodeConfigValues(inputNodeCfg, conf) (it saves the merged config to the database). But only after that the DataDir is joined with the conf.rootDataDir. Some tests might fail because they contain "../static/test-account" in the DataDir (TestLoginAndMigrationsStillWorkWithExistingDesktopUser).

Implementation

Implement #5563, reuse #5527, fix tests

Notes

from #5527 (review)

But I'm pretty sure there will be problems. It affects many values and it was working before somehow.

friofry added a commit to status-im/status-desktop that referenced this issue Jul 26, 2024
Fixes #14929

Correct fix should be done as part of status-im/status-go#5597 after dependency tasks are done
@jrainville jrainville added this to the 2.31.0 Beta milestone Jul 26, 2024
friofry added a commit to status-im/status-desktop that referenced this issue Jul 26, 2024
Fixes #14929

Correct fix should be done as part of status-im/status-go#5597 after dependency tasks are done
friofry added a commit to status-im/status-desktop that referenced this issue Jul 26, 2024
Fixes #14929

Correct fix should be done as part of status-im/status-go#5597 after dependency tasks are done

(cherry picked from commit ac04e34)
jrainville pushed a commit to status-im/status-desktop that referenced this issue Jul 29, 2024
#15838)

Fixes #14929

Correct fix should be done as part of status-im/status-go#5597 after dependency tasks are done

(cherry picked from commit ac04e34)
@igor-sirotin igor-sirotin added bug NodeConfig Anything related to NodeConfig labels Aug 29, 2024
@jrainville jrainville modified the milestones: 2.31.0 Beta, 2.32.0 Beta Sep 5, 2024
@igor-sirotin
Copy link
Collaborator

I think it's time to fix this. I'm facing another issue trying to pass TestNetworksOverride in CreateAccountAndLogin:

TestOverrideNetworks []params.Network `json:"-"` // This is used for testing purposes only

It works on the first run, but on the next Login it overwrites the saved networks with the defaults.

@friofry wanna get back to this issue and try to fix it again? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core-team NodeConfig Anything related to NodeConfig
Projects
Status: No status
Development

No branches or pull requests

3 participants