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

NodeConfig, make sure that absolute paths are saved #5563

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

NodeConfig, make sure that absolute paths are saved #5563

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

Comments

@friofry
Copy link
Contributor

friofry commented Jul 23, 2024

Problem

in some places DataDir is expected to be relative path

 loadNodeConfig:
    conf.RootDataDir = b.rootDataDir
    conf.DataDir = filepath.Join(b.rootDataDir, conf.DataDir)
    conf.KeyStoreDir = filepath.Join(b.rootDataDir, conf.KeyStoreDir)

or

func defaultNodeConfig(installationID string) (*params.NodeConfig, error) {
...
	nodeConfig.DataDir = api.DefaultDataDir
}

and other palces expect absolute paths:

NewNodeConfig:
    config := &NodeConfig{
        NetworkID:              networkID,
        RootDataDir:            dataDir,
        DataDir:                dataDir,

We need to make sure that all paths in NodeConfig are stored in absolute form (or if that's not possible, relative, but it should be consistent).
And the code that concatenates absolute paths should be fixed conf.DataDir = filepath.Join(b.rootDataDir, conf.DataDir)

(it was found while I was debugging tests in #5527)

Implementation

DataDir, KeyStoreDir, RootDataDir, LogDir, LogFile should be absolute

Functions like getDefaultDataDir should be used correctly. And names of fields and methods should reflect the path type (perhaps it makes sense to introduce a wrapper type for FilePath to support both forms).

Acceptance Criteria

See status-im/status-mobile#9942

[status-im/status-react#9942] Upgradable paths in configs

Storing absolute path for different configs breaks compatibility on iOS
as app's dir is changed after upgrade. The solution is to store relative
paths and to concatenate it with `backend.rootDataDir`. The only
exception is `LogFile` as it is stored outside `backend.rootDataDir` on
Android. `LogDir` config was added to allow adding of custom dir for log
file.
Configs concerned:
`DataDir`
`LogDir`
`LogFile`
`KeystoreDir`
`BackupDisabledDataDir`

We need to make sure that mobile handles this correctly.

@qfrank
Copy link
Contributor

qfrank commented Jul 24, 2024

[status-im/status-react#9942] Upgradable paths in configs

Storing absolute path for different configs breaks compatibility on iOS
as app's dir is changed after upgrade. The solution is to store relative
paths and to concatenate it with `backend.rootDataDir`. The only
exception is `LogFile` as it is stored outside `backend.rootDataDir` on
Android. `LogDir` config was added to allow adding of custom dir for log
file.
Configs concerned:
`DataDir`
`LogDir`
`LogFile`
`KeystoreDir`
`BackupDisabledDataDir`

thanks for the info, so I think our only solution would be make sure that all paths in NodeConfig are stored in relative path? and this was also my old understanding, but I didn't expect it was not consistent 🙂 @friofry

@igor-sirotin igor-sirotin added bug NodeConfig Anything related to NodeConfig labels Aug 29, 2024
@jrainville jrainville modified the milestones: 2.31.0, 2.32.0 Nov 29, 2024
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 refactor
Projects
Status: No status
Development

No branches or pull requests

4 participants