Correct dockerfiles to run the tests successfully again. split prepa… #103

devinleighsmith · 2021-04-26T17:40:13Z

…re script to allow better caching.

Signed-off-by: Devin Smith devinleighsmith@gmail.com

devinleighsmith · 2021-04-26T19:22:48Z

Based off of #101

Primary motivations:

fix issues preventing existing tests from running.
split prepare script up to allow created dockerfiles to be individually cached.
update sovrin dependency to allow it to be installed via the GHA within the sovrin repo (by default sovrin is not required in these tests).

devinleighsmith · 2021-04-26T19:23:14Z

system/docker/client/Dockerfile

-        setuptools \
-        virtualenv==16.0.0 \
-        pipenv==2018.11.26 \
+    pip==10.0.0 \


required fix for breaking tests.

devinleighsmith · 2021-04-26T19:23:37Z

system/docker/client/Dockerfile

+RUN : ${LIBINDY_VERSION:?"LIBINDY_VERSION must be provided"}
+ENV LIBINDY_VERSION=${LIBINDY_VERSION}
+
+RUN apt-get install -y \


required fix for breaking tests.

devinleighsmith · 2021-04-26T19:24:18Z

system/docker/client/Dockerfile

-ARG LIBINDY_VERSION
-ENV LIBINDY_REPO_COMPONENT=${LIBINDY_REPO_COMPONENT:-master}
-ENV LIBINDY_VERSION=${LIBINDY_VERSION:-1.10.0~1171}
+# sovrin repo init


allow greater degree of control over dockerfiles via env variables (the GHA for sovrin uses these, for example).

devinleighsmith · 2021-04-26T19:25:02Z

system/docker/node/Dockerfile

 # Install environment
 RUN apt-get update -y && apt-get install -y \
-        git \
-        wget \
-        python3.5 \


3.5 installed by default in xenial.

devinleighsmith · 2021-04-26T19:26:05Z

system/docker/node/Dockerfile

 ARG LIBINDY_CRYPTO_VERSION
+RUN : ${LIBINDY_CRYPTO_VERSION:?"LIBINDY_CRYPTO_VERSION must be provided"}


provides greater control and better error feedback when using env vars to control the test behaviour.

devinleighsmith · 2021-04-26T19:27:59Z

system/docker/node/Dockerfile

-        rm -rf /var/lib/apt/lists/*; \
+COPY . . 
+RUN if [ -n "$SOVRIN_INSTALL" ]; then \
+    dpkg -i "sovrin_${SOVRIN_VERSION}_amd64.deb"; \


this is what installs the pending sovrin package in the GHA.

alternative would be to publish a dev sovrin deb every time a pull request is created, but not sure if that will be something the sovrin-foundation nexus would/should support @WadeBarnes

The comment below says that we gonna cache both images, but can we cache the node image since it includes the built source?

the github actions responsible for caching can cache based on a hash of changed files. The hash can include multiple file patterns. So if we want node to rebuild whenever the source changes, we could do something like
**/Dockerfile, **.*.py

Ok, fine. I just think .py files will be changed in most commits so the cache can make the build process even slower in this case.

But anyway, it's fine for the first version.

devinleighsmith · 2021-04-26T19:29:13Z

system/docker/prepare_client.sh

@@ -0,0 +1,87 @@
+#!/bin/bash


as mentioned, splitting the above prepare script into these two components allow the GHA to build/cache the node and client docker images separately.

WadeBarnes · 2021-04-26T21:06:36Z

@askolesov, @Toktar, Could you please review and merge?

askolesov

Looks good. The only thought - we shouldn't cache node image since it includes the built sources. Probably we could split it into two images and cache the part without sources. Anyway, it's not about this repo and this PR.

Toktar · 2021-04-28T08:34:00Z

system/docker/client/Dockerfile

    && rm -rf /var/lib/apt/lists/*

 # python
-ARG PYTHON3_VERSION
-ENV PYTHON3_VERSION=${PYTHON3_VERSION:-3.5}


Could you please tell why the python version is being removed here? Soon it is planned to migrate to ubuntu20.04 and this variable may be needed due to the fact that the version of python will not coincide with the default version.
Of course, this is only relevant for the case when this Dockerfile will be edited to create an image for ubuntu20.04, and not a new one will be created from scratch.

Right, the idea was to remove this since 3.5 is installed by default on xenial. You are also correct that if this was bumped to 20.04 that would cause this dockerfile to use 3.8 by default.

I think in the ideal case we use 3.8 if possible anyways when this is moved to 20.04, since I don't think we want to use 3.5 with 20.04 as it is no longer supported. So I think even with the update to 20.04 we would prefer to use the default python installation that comes with 20.04. If for some reason we aren't able to move to 3.8 we could add this back.

Is it desirable for someone running the tests to be able to switch python versions on the fly? In my experience so far it seems like everything is running on 3.5 currently, and with the move to 20.04 it seems likely that everything will run on 3.8. Is there a need for these tests to be run on arbitrary versions of python? If so I should probably put this back

Given that we are using the default versions on both systems, I think you are right. We can remove this variable.
I hope that we will write system tests for upgrading from Ubuntu 16.04 to Ubuntu 20.04 and there will be a change of python version on the fly, but the case is specific. Then we can return this variable if needed.

Toktar · 2021-04-29T09:39:30Z

Could you please fix DCO sign for commit 9f1d91f and resolve conflicts.
Changes look excellent. I'll approve them after fixes.

…e script to allow better caching. Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

devinleighsmith · 2021-04-30T18:20:03Z

@Toktar @askolesov I had to make a few updates to get 100% of the tests to pass, viewable here: https://github.com/devinleighsmith/sovrin/runs/2468937322?check_suite_focus=true

should be good for approval now.

WadeBarnes · 2021-05-10T14:50:50Z

@Toktar @askolesov, are you able to provide a final review, then approve and merge?

devinleighsmith requested review from lampkin-diet, ashcherbakov, skhoroshavin, Toktar and VladimirWork as code owners April 26, 2021 17:40

devinleighsmith commented Apr 26, 2021

View reviewed changes

devinleighsmith force-pushed the test_fix branch from 9801eca to 0a4b695 Compare April 26, 2021 21:00

askolesov previously approved these changes Apr 28, 2021

View reviewed changes

Toktar reviewed Apr 28, 2021

View reviewed changes

devinleighsmith dismissed askolesov’s stale review via 7654ad7 April 29, 2021 05:13

devinleighsmith force-pushed the test_fix branch 2 times, most recently from 7654ad7 to 8d3f8ee Compare April 29, 2021 05:22

devinleighsmith force-pushed the test_fix branch 3 times, most recently from 65282da to 4f419d3 Compare April 29, 2021 15:40

devinleighsmith added 3 commits April 29, 2021 08:46

Correct dockerfiles to run the tests successfully again. split prepar…

e13b69a

…e script to allow better caching. Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

make use of optional installs consistent.

d36ccd9

Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

update to support ursa.

49ea681

Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

devinleighsmith force-pushed the test_fix branch from 4f419d3 to 49ea681 Compare April 29, 2021 15:47

bump version of libindy python wrapper.

f58d8c8

Signed-off-by: Devin Smith <devinleighsmith@gmail.com>

devinleighsmith force-pushed the test_fix branch from b18b7aa to f58d8c8 Compare April 29, 2021 16:55

Toktar approved these changes May 12, 2021

View reviewed changes

Toktar merged commit 11e8f96 into hyperledger:master May 12, 2021

WadeBarnes mentioned this pull request Sep 14, 2021

Indy Node system tests depend on the Sovrin package #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct dockerfiles to run the tests successfully again. split prepa… #103

Correct dockerfiles to run the tests successfully again. split prepa… #103

devinleighsmith commented Apr 26, 2021

devinleighsmith commented Apr 26, 2021

devinleighsmith Apr 26, 2021

devinleighsmith Apr 26, 2021

devinleighsmith Apr 26, 2021

devinleighsmith Apr 26, 2021

devinleighsmith Apr 26, 2021

devinleighsmith Apr 26, 2021

askolesov Apr 28, 2021

devinleighsmith Apr 28, 2021

askolesov Apr 28, 2021

askolesov Apr 28, 2021

devinleighsmith Apr 26, 2021

WadeBarnes commented Apr 26, 2021

askolesov left a comment

Toktar Apr 28, 2021

devinleighsmith Apr 28, 2021

Toktar Apr 29, 2021

Toktar commented Apr 29, 2021

devinleighsmith commented Apr 30, 2021

WadeBarnes commented May 10, 2021

		ARG LIBINDY_CRYPTO_VERSION
		RUN : ${LIBINDY_CRYPTO_VERSION:?"LIBINDY_CRYPTO_VERSION must be provided"}

Correct dockerfiles to run the tests successfully again. split prepa… #103

Correct dockerfiles to run the tests successfully again. split prepa… #103

Conversation

devinleighsmith commented Apr 26, 2021

devinleighsmith commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WadeBarnes commented Apr 26, 2021

askolesov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toktar commented Apr 29, 2021

devinleighsmith commented Apr 30, 2021

WadeBarnes commented May 10, 2021