-
Notifications
You must be signed in to change notification settings - Fork 42
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
Small PR to address missing content for docker builds #461
Conversation
Several changes to ping test to make it a little more useful: * Removed the dependencies on PDO configuration including configuration files and environment variables; everything gets specified as command line parameters: ping_test.py --url ${PDO_LEDGER_URL} --cert ${PDO_LEDGER_KEY_ROOT}/networkcert.pem * Allow for "quiet" mode operation so that the result code can be used in other scripts. * Change behavior on failure to print the root cause message for an exception that occurs; CCF was hiding the useful messages Signed-off-by: Mic Bowman <[email protected]>
* add lsof to the packages to install for ccf; the test for duplicate services was failing (non-fatally); this should fix it * add explicit setting of noproxy in start_services to mirror handling in other tools scripts. this fixes a problem when running persistent test services on localhost. Signed-off-by: Mic Bowman <[email protected]>
651bbed
to
23acb88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well in my tests. There are a few docker issues i encountered but i will address them in a separate PR.
One other thing i run into is the authentication failures with ping_test. After i knew issue that i have to use IP addresses, i re-read docu and noticed that you do mention that ip-addresses should be used. But could be worth mentioning the failures one would encounter if not doing so?
fi | ||
|
||
export no_proxy=$PDO_HOSTNAME,$no_proxy | ||
export NO_PROXY=$PDO_HOSTNAME,$NO_PROXY | ||
export no_proxy=$PDO_HOSTNAME,$PDO_LEDGER_ADDRESS,$no_proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already doing it for PDO_HOSTNAME and for most folks this will result with the "right" thing but for somebody having a proxy and one of the services outside of proxy, this could lead to unexpected behaviour given how hidden this is here? Don't immediately have a good alternative but we could mention it in the ready and/or add a log output here? No strong feeling in leaving it as is, though, either ...
@@ -131,7 +131,7 @@ at the end of the test. | |||
|
|||
```bash | |||
source $PDO_HOME/ccf/bin/activate | |||
${PDO_SOURCE_ROOT}/ledgers/ccf/scripts/ping_test.py | |||
${PDO_SOURCE_ROOT}/ledgers/ccf/scripts/ping_test.py --url ${PDO_LEDGER_URL} --cert ${PDO_LEDGER_KEY_ROOT}/networkcert.pem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing, i noticed that PDO_LEDGER_KEY_ROOT is not defined in client docker image. There are though also a few other docker stuff i've noticed, will address in separate PR ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run start_client.sh... it will invoke environment.sh which sets all variables.
start_client.sh plays an explicit role of .bashrc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, looks good
Three things in this PR:
anywhere