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

Save only the settings that are changed #14930

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

friofry
Copy link
Contributor

@friofry friofry commented May 27, 2024

What does the PR do

status-go PR status-im/status-go#5237
fixes #12918 #14889

  1. The main goal - remove all uses of saveNodeConfiguration. Because it can overwrite the values it doesn't know about.
    (for example Fix(settings): crash when enabling history archive and disabling logs #14812)

  2. Use instead: setLightClient, saveNewWakuNode, setMaxLogBackups (How many log files to keep archived setting is set to 0 #14889)

Affected areas

Settings, NodeConfig

Screenshot of functionality (including design for comparison)

settings.mp4

additional info

setNimbusProxyConfigEnabled currently is not used anywhere. Should be fixed after merging status-im/status-go#4254

further improvements:

#14929

@friofry friofry requested review from jrainville, richard-ramos, kounkou and a team and removed request for jrainville, richard-ramos and kounkou May 27, 2024 13:52
@friofry friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch from 3912dbb to 32d14e3 Compare May 27, 2024 13:53
@caybro caybro requested a review from Seitseman May 27, 2024 13:59
@caybro
Copy link
Member

caybro commented May 27, 2024

@Seitseman check this out pls, I have a guts feeling this might actually also fix status-im/status-go#5186

@status-im-auto
Copy link
Member

status-im-auto commented May 27, 2024

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 32d14e3 #2 2024-05-27 14:00:51 ~6 min macos/aarch64 🍎dmg
✔️ 32d14e3 #2 2024-05-27 14:01:35 ~7 min tests/nim 📄log
✔️ 32d14e3 #2 2024-05-27 14:03:59 ~9 min tests/ui 📄log
✔️ 32d14e3 #2 2024-05-27 14:04:36 ~10 min macos/x86_64 🍎dmg
32d14e3 #2 2024-05-27 14:05:08 ~11 min windows/x86_64 📄log
✔️ 32d14e3 #2 2024-05-27 14:11:15 ~17 min linux/x86_64 📦tgz
a266755 #3 2024-05-30 11:40:59 ~2 min macos/aarch64 📄log
a266755 #3 2024-05-30 11:42:22 ~3 min linux/x86_64 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
66189dc #4 2024-05-30 11:43:59 ~1 min linux/x86_64 📄log
✔️ 66189dc #4 2024-05-30 11:50:26 ~7 min macos/aarch64 🍎dmg
✔️ 66189dc #4 2024-05-30 11:50:39 ~8 min tests/nim 📄log
✔️ 66189dc #4 2024-05-30 11:53:11 ~10 min macos/x86_64 🍎dmg
✔️ 66189dc #4 2024-05-30 11:54:04 ~11 min tests/ui 📄log
✔️ 66189dc #4 2024-05-30 12:06:54 ~24 min windows/x86_64 💿exe
✔️ f246859 #5 2024-06-10 14:38:21 ~4 min tests/nim 📄log
✔️ f246859 #5 2024-06-10 14:39:55 ~6 min macos/aarch64 🍎dmg
✔️ f246859 #5 2024-06-10 14:45:37 ~12 min tests/ui 📄log
✔️ f246859 #5 2024-06-10 14:45:47 ~12 min macos/x86_64 🍎dmg
✔️ f246859 #5 2024-06-10 14:50:14 ~16 min linux/x86_64 📦tgz
✔️ f246859 #5 2024-06-10 15:05:06 ~31 min windows/x86_64 💿exe

@friofry
Copy link
Contributor Author

friofry commented May 27, 2024

@Seitseman check this out pls, I have a guts feeling this might actually also fix status-im/status-go#5186

yeah, also this improvement might fix the problem: #14813

Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@igor-sirotin
Copy link
Contributor

FYI, could be related: #12918

@@ -122,7 +122,7 @@ proc isNimbusProxyEnabled*(self: Controller): bool =
proc toggleNimbusProxy*(self: Controller) =
let enabled = self.nodeConfigurationService.isNimbusProxyEnabled()

if(not self.nodeConfigurationService.setNimbusProxyConfig(not enabled)):
if not self.nodeConfigurationService.setNimbusProxyConfigEnabled(not enabled):
Copy link
Contributor

@kounkou kounkou May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setNimbusProxyConfigEnabled is a setter that returns a boolean, what does it mean 😲 ? See comment below.

Copy link
Contributor Author

@friofry friofry May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a setter that returns a boolean, what does it mean

Sorry, not sure what you mean. I thought you had added this code. But I guess the purpose here is to handle status-go errors for whatever reason and log it

@Seitseman
Copy link
Contributor

This PR indeed fixes the problem with Windows not logging in. LGTM

@jrainville
Copy link
Member

This PR indeed fixes the problem with Windows not logging in. LGTM

Awesome! @friofry then we'll need to cherry-pick this PR to the release branch after

@Seitseman
Copy link
Contributor

Seitseman commented May 28, 2024

Well, it looks like when I want to sync "this" app with other, and there is no data folder, status-im/status-go#5186 would still occur. And when I'm creating new user, it will still error out. So this PR is not fixing 5255

@jrainville
Copy link
Member

Well, it looks like when I want to sync "this" app with other, and there is no data folder, status-im/status-go#5186 would still occur. And when I'm creating new user, it will still error out. So this PR is not fixing #5186

Ah ok. Then @friofry it's not necessary to cherry-pick

* fix saveNewWakuNode in the settings
* fix setNimbusProxyConfigEnabled in the settings
* fix setLightMode in the settings
@friofry friofry force-pushed the ab/issue-12918-setting-update-with-diffs branch from 66189dc to f246859 Compare June 10, 2024 14:33
@friofry friofry merged commit 4145be8 into master Jun 10, 2024
8 checks passed
@friofry friofry deleted the ab/issue-12918-setting-update-with-diffs branch June 10, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not modify the node configuration directly
7 participants