Lagrange-Labs / lsc-client-cli

Other
13 stars 5 forks source link

chore: ensure config file path #93

Closed mastereng12 closed 3 months ago

mastereng12 commented 3 months ago

Context

Closes: #92


Reviewers

kashishshah commented 3 months ago

What happens in a scenario when the command like generate-config is run from root of the package client-cli and the operator only passes -c config.toml?

I think in dirName, fileName := filepath.Split(configFilePath) when dirName is empty string, we need to set dirName="."

Wdyt @mastereng12?

mastereng12 commented 3 months ago

What happens in a scenario when the command like generate-config is run from root of the package client-cli and the operator only passes -c config.toml?

I think in dirName, fileName := filepath.Split(configFilePath) when dirName is empty string, we need to set dirName="."

Wdyt @mastereng12?

The path would be translated to the absolute path, you don't need to care about the dirname, the purpose of this PR is to ensure the config file is loaded properly. If not, then it will be panic, so the users can recognize the wrong path.

kashishshah commented 3 months ago

What happens in a scenario when the command like generate-config is run from root of the package client-cli and the operator only passes -c config.toml? I think in dirName, fileName := filepath.Split(configFilePath) when dirName is empty string, we need to set dirName="." Wdyt @mastereng12?

The path would be translated to the absolute path, you don't need to care about the dirname, the purpose of this PR is to ensure the config file is loaded properly. If not, then it will be panic, so the users can recognize the wrong path.

Please check the below error

~/client-cli$ ./dist/lagrange-cli generate-config -c config.toml -n holesky -r arbitrum
FATA[2024-06-12T13:07:14.365Z] failed to load CLI config: operator keystore path is required
Stack trace: 
/home/ubuntu/go/pkg/mod/github.com/!lagrange-!labs/lagrange-node@v0.4.0/logger/logger.go:46 github.com/Lagrange-Labs/lagrange-node/logger.appendStackTraceMaybeArgs()
/home/ubuntu/go/pkg/mod/github.com/!lagrange-!labs/lagrange-node@v0.4.0/logger/logger.go:92 github.com/Lagrange-Labs/lagrange-node/logger.Fatal()
/home/ubuntu/client-cli/cmd/main.go:269 main.main()
/usr/local/go/src/runtime/proc.go:267 runtime.main() 
mastereng12 commented 3 months ago

@kashishshah , did you test against the current branch? In my case, failed to load CLI config: config file not found: Config File "config" Not Found in "[]"

kashishshah commented 3 months ago

@kashishshah , did you test against the current branch? In my case, failed to load CLI config: config file not found: Config File "config" Not Found in "[]"

@mastereng12 The error log I pasted was for the existing version v0.2.3. For the fix/config_path branch, I get the error log that you posted but even that should not be the case right? The devs don't mention ./config.toml when they are running from the root of the client-cli thinking that it will pick up config.toml as it is present in that directory

mastereng12 commented 3 months ago

@kashishshah , did you test against the current branch? In my case, failed to load CLI config: config file not found: Config File "config" Not Found in "[]"

@mastereng12 The error log I pasted was for the existing version v0.2.3. For the fix/config_path branch, I get the error log that you posted but even that should not be the case right? The devs don't mention ./config.toml when they are running from the root of the client-cli thinking that it will pick up config.toml as it is present in that directory

sorry, I don't understand your point, firstly the default value is set as ./config.toml, generally the file path should be absolute or relative. Secondly it will be panic with the above error if users parse the wrong path. I don't think we need to provide the ambiguous path format like -c config.toml.

kashishshah commented 3 months ago

@kashishshah , did you test against the current branch? In my case, failed to load CLI config: config file not found: Config File "config" Not Found in "[]"

@mastereng12 The error log I pasted was for the existing version v0.2.3. For the fix/config_path branch, I get the error log that you posted but even that should not be the case right? The devs don't mention ./config.toml when they are running from the root of the client-cli thinking that it will pick up config.toml as it is present in that directory

sorry, I don't understand your point, firstly the default value is set as ./config.toml, generally the file path should be absolute or relative. Secondly it will be panic with the above error if users parse the wrong path. I don't think we need to provide the ambiguous path format like -c config.toml.

Your point is valid and I agree to it (as an engineer) but I have seen many operators using -c config.toml as they run the command from root and config.toml is also present at the same path which leads to an error. They reach out to us for this so I want to avoid this if possible

kashishshah commented 3 months ago

@mastereng12 If you want to avoid making that change then maybe we can modify the error log to state the path and also mention to use absolute or relative path.