Add the CLI param "-dbURI" #4385
Conversation
|
I have fixed errors for E2E tests. |
| const char* pwd = strstr(dbURI, "${PWD}"); | ||
| if (pwd != NULL) | ||
| { | ||
| if (strlen(passwd) == 0) | ||
| { | ||
| LM_X(1, ("Invalid Command Line Options: -dbURI is used with a password substitution, but no password (-dbpwd) is supplied")); | ||
| } | ||
|
|
||
| uri = std::string(dbURI, pwd - dbURI) + passwd + (pwd + 6); | ||
| } |
There was a problem hiding this comment.
Why implementing this password substitution mechanism? Why don't just use the password in the -dbUri parameter? Could you elaborate on this?
There was a problem hiding this comment.
This is because Orion-LD implements that functionality.
See also following comment:
#3794 (comment)
There was a problem hiding this comment.
After reading the documetnation added to cli.md, now it's clear to me how it works :)
It can be an interesting feature. Let's keep it.
NTC
| @@ -148,6 +148,7 @@ char authMech[64]; | |||
| char authDb[64]; | |||
There was a problem hiding this comment.
CHANGES_NEXT_RELEASE should include the changes in this PR. Maybe something like this:
- Add: CLI parameter -dbUri / env var ORION_MONGO_URI (#3794)
There was a problem hiding this comment.
In addition, documetnation in cli.md should be modified to describe the new CLI and env var
|
|
||
| // Add auth parameter if included | ||
| if (strlen(username) != 0 && strlen(passwd) != 0) | ||
| if (strlen(dbURI) != 0) |
There was a problem hiding this comment.
If -dbUri is used at the same time some of the old parameters (-dbhost, etc.) I understand that -dbUri will take preference.
That's fine, but such case of combining old and new CLI would be probably a misconfiguration that should be at least warned to the user (or maybe even better, raise a LM_X so the user have the chance to fix the configuration).
Co-authored-by: Fermín Galán Márquez <[email protected]>
Co-authored-by: Fermín Galán Márquez <[email protected]>
|
PR looks nice! :) However, there is a small conflict in CHANGES_NEXT_RELEASE that need to be solved before merging. Could you do it, please? In addition, I'm afraid I don't understand Japanese, so I cannot evaluate that parts in the PR. Maybe @fisuda you could help having a look and providing feeback/LGTM, please? Thanks! |
|
I have fixed the conflict in CHANGES_NEXT_RELEASE. |
PR #3794