Skip to content
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

fix(test-tooling): getContainerInfo methods lookup criteria #265

Closed
petermetz opened this issue Aug 27, 2020 · 0 comments · Fixed by #283
Closed

fix(test-tooling): getContainerInfo methods lookup criteria #265

petermetz opened this issue Aug 27, 2020 · 0 comments · Fixed by #283
Labels
bug Something isn't working good-first-issue Good for newcomers
Milestone

Comments

@petermetz
Copy link
Member

Describe the bug

Currently all the test ledger implementation classes have a method like this (or similar) that look up the information about their own container by selecting it from the list of all running containers based on the image name.
This works when there's only ever a single container of that image is running but breaks when there are 2 or more containers with the same image (when tests are running in parallel for example).

To Reproduce

Read the code here and observe the issue:

As seen in the test ledger classes:

  protected async getContainerInfo(): Promise<ContainerInfo> {
    const fnTag = "FabricTestLedgerV1#getContainerInfo()";
    const docker = new Docker();
    const image = this.getContainerImageName();
    const containerInfos = await docker.listContainers({});

    const aContainerInfo = containerInfos.find((ci) => ci.Image === image);

    if (aContainerInfo) {
      return aContainerInfo;
    } else {
      throw new Error(`${fnTag} no image "${image}"`);
    }
  }

Expected behavior

Container look ups are done in a way that makes it impossible to return the wrong container, e.g. based on the container ID for example.
When the container gets started/created, the test ledger class should save the resulting ID of it as a pointer to the actual container.

Also, it would be nice to pull this implementation to a more generic place such as a the Containers helper class and maybe rename it at the same time to something like getContainerInfoById(id: string): Promise<ContainerInfo>.

Hyperledger Cactus release version or commit (git rev-parse --short HEAD):

769c087

cc: @jonathan-m-hamilton @sfuji822 @takeutak @jweate @suyukuoacn @RafaelAPB

@petermetz petermetz added bug Something isn't working good-first-issue Good for newcomers labels Aug 27, 2020
suyukuoacn added a commit to suyukuoacn/cactus that referenced this issue Sep 15, 2020
Fixes hyperledger#265

Signed-off-by: suyukuoacn <su-yu.kuo@accenture.com>
suyukuoacn added a commit to suyukuoacn/cactus that referenced this issue Sep 15, 2020
Fixes hyperledger#265

Signed-off-by: suyukuoacn <su-yu.kuo@accenture.com>
@petermetz petermetz added this to the v0.2.0 milestone Sep 25, 2020
petermetz pushed a commit that referenced this issue Oct 6, 2020
Fixes #265

Signed-off-by: suyukuoacn <su-yu.kuo@accenture.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good-first-issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant