-
Notifications
You must be signed in to change notification settings - Fork 10
[docker-test] TLS support in the all-in-one test image node with external connectivity #190
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
[docker-test] TLS support in the all-in-one test image node with external connectivity #190
Conversation
…s TLS modes and using external connections with full transaction cycle + querying Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
mbrandenburger
left a comment
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 @dean-amar - great PR!
docker/test/common.go
Outdated
| testNodeImage = "icr.io/cbdc/committer-test-node:0.0.2" | ||
| channelName = "mychannel" | ||
| monitoredMetric = "loadgen_transaction_committed_total" | ||
| testNodeImage = "icr.io/cbdc/committer-test-node:0.0.2" |
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.
I am wondering if there is a better way t set the name of the image based on whatever tag are? Maybe latests and let the make script either build?
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 can simplify this by making the image tag configurable via the Makefile.
In the Makefile, we will change the version to a mutable variable:
version ?= 0.0.2
export TEST_NODE_TAG := $(version)
Then, in the tests, we derive the full image name based on the tag:
var testNodeImage = func() string {
base := "icr.io/cbdc/committer-test-node:"
if tag := os.Getenv("TEST_NODE_TAG"); tag != "" {
return base + tag
}
return base + "latest"
}()
Would that work?
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.
Yes, this would be good.
Looking at the makefile, we also use the ${image_namespace} to build the image name. I guess we should also consider this in the test?
build-test-node-image: build-arch build-test-genesis-block
${docker_cmd} build $(docker_build_flags) \
-f $(dockerfile_test_node_dir)/Dockerfile \
-t ${image_namespace}/committer-test-node:${version} \
--build-arg ARCHBIN_PATH=${arch_output_dir_rel} \
. $(docker_push_arg)Wouldn't it be simpler to just define the full name in the makefile and export it, so we can use it's name via the env variable in the test.
For example:
test_node_image_full ?= ${image_namespace}/committer-test-node:${version}
build-test-node-image: build-arch build-test-genesis-block
${docker_cmd} build $(docker_build_flags) \
-f $(dockerfile_test_node_dir)/Dockerfile \
-t ${test_node_image_full} \
--build-arg ARCHBIN_PATH=${arch_output_dir_rel} \
. $(docker_push_arg)
And as you suggested in the test ...
var testNodeImage = func() string {
default := "icr.io/cbdc/committer-test-node:latest" // not sure if this default is reasonable
return cmp.Or(os.Getenv("test_node_image_full"), default)
}()WDYT?
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.
Yeah, that's great! I'll add it to the next commit.
liran-funaro
left a comment
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.
LGTM. Mostly minor comments.
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
…E.md file. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
mbrandenburger
left a comment
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.
LGTM
Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
* Revert makefile's changes until we clear up Marcus usage intentions. Signed-off-by: Dean Amar <Dean.Amar@ibm.com>
Type of change
Description
Related issues