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

test(test tooling): add DamlTestLedger implementation #3462

Merged

Conversation

raynatopedrajeta
Copy link
Contributor

@raynatopedrajeta raynatopedrajeta commented Aug 7, 2024

Commit to be reviewed

test(test tooling): add DamlTestLedger implementation

Primary Changes
---------------
1. Create a test tooling class for DAML AIO Image

Fixes #3435

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@jagpreetsinghsasan
Copy link
Contributor

@raynatopedrajeta please remove the daml connector folder (containing the package.json) and fix the failing CI issues

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raynatopedrajeta Plus one to what @jagpreetsinghsasan said plus the usual nits like squashing the commits and having a PR title + commit subject that isn't going to end up in the production changelog.md file (feat will make it so that it will so it needs to be test even if it is technically a feature of the testing capabilities)

@jagpreetsinghsasan
Copy link
Contributor

@raynatopedrajeta Plus one to what @jagpreetsinghsasan said plus the usual nits like squashing the commits and having a PR title + commit subject that isn't going to end up in the production changelog.md file (feat will make it so that it will so it needs to be test even if it is technically a feature of the testing capabilities)

True @petermetz , although Rayn has 2 commits in this PR as one of the commits if of the previous PR (This PR is in continuation to #3411 , thus 2 commits)

@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 4 times, most recently from 2c01942 to 11847b2 Compare August 13, 2024 13:26
Copy link
Contributor

@RafaelAPB RafaelAPB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. I've left a few comments. Thanks for your contribution!

@raynatopedrajeta raynatopedrajeta marked this pull request as draft August 15, 2024 06:40
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 6 times, most recently from cca6931 to bf06a08 Compare August 20, 2024 06:44
@raynatopedrajeta raynatopedrajeta marked this pull request as ready for review August 20, 2024 06:56
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 6 times, most recently from ef4e15d to 8c317eb Compare August 20, 2024 07:36
@raynatopedrajeta
Copy link
Contributor Author

@raynatopedrajeta please fix the failing CI tests

Hello @jagpreetsinghsasan ,

Review point has been addressed.

Cheers!
Rayn

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raynatopedrajeta I'll publish a build of the image from the PR on the official repo shortly. Could you please then update the source code to use that image instead?
Also, could you remove the entry "cactuts" from the spell checker configuration? I'm assuming it was a typo while typing "cacti" so we shouldn't add the typo itself in the list of correct words.

@petermetz
Copy link
Contributor

Suggested edit:

diff --git a/packages/cactus-plugin-ledger-connector-daml/src/test/typescript/integration/daml-get-transaction.test.ts b/packages/cactus-plugin-ledger-connector-daml/src/test/typescript/integration/daml-get-transaction.test.ts
index 490c5ff23..b4d3fb42d 100644
--- a/packages/cactus-plugin-ledger-connector-daml/src/test/typescript/integration/daml-get-transaction.test.ts
+++ b/packages/cactus-plugin-ledger-connector-daml/src/test/typescript/integration/daml-get-transaction.test.ts
@@ -6,11 +6,10 @@ import {
   LoggerProvider,
   Logger,
 } from "@hyperledger/cactus-common";
+
+import { DAML_TEST_LEDGER_DEFAULT_OPTIONS } from "@hyperledger/cactus-test-tooling";
 import { pruneDockerAllIfGithubAction } from "../../../../../../packages/cactus-test-tooling/src/main/typescript/github-actions/prune-docker-all-if-github-action";
 //Constants
-const containerImageVersion = "latest";
-const containerImageName = "cactuts/daml-all-in-one";
-const rpcApiHttpPort = 7575;
 
 const testLogLevel: LogLevelDesc = "info";
 
@@ -28,12 +27,12 @@ describe("PluginLedgerConnectorDAML", () => {
     await pruneDockerAllIfGithubAction({ logLevel: testLogLevel });
 
     damlTestLedger = new DamlTestLedger({
-      containerImageVersion,
-      containerImageName,
-      rpcApiHttpPort,
+      imageName: DAML_TEST_LEDGER_DEFAULT_OPTIONS.imageName,
+      imageVersion: DAML_TEST_LEDGER_DEFAULT_OPTIONS.imageVersion,
+      rpcApiHttpPort: DAML_TEST_LEDGER_DEFAULT_OPTIONS.rpcApiHttpPort,
     });
 
-    log.debug("DAML image:", damlTestLedger.containerImageName);
+    log.debug("DAML image:", damlTestLedger.imageName);
     expect(damlTestLedger).toBeTruthy();
     await damlTestLedger.start();
   });
diff --git a/packages/cactus-test-tooling/src/main/typescript/daml/daml-test-ledger.ts b/packages/cactus-test-tooling/src/main/typescript/daml/daml-test-ledger.ts
index f1d6ea253..fbe666c8b 100644
--- a/packages/cactus-test-tooling/src/main/typescript/daml/daml-test-ledger.ts
+++ b/packages/cactus-test-tooling/src/main/typescript/daml/daml-test-ledger.ts
@@ -12,24 +12,26 @@ import { ITestLedger } from "../i-test-ledger";
 import { Streams } from "../common/streams";
 import { Containers } from "../common/containers";
 
-export interface DAMLTestLedgerConstructorOptions {
-  containerImageVersion?: string;
-  containerImageName?: string;
+export interface IDamlTestLedgerOptions {
+  imageVersion?: string;
+  imageName?: string;
   rpcApiHttpPort?: number;
   logLevel?: LogLevelDesc;
   emitContainerLogs?: boolean;
 }
 
-export const DAML_TEST_LEDGER_DEFAULT_OPTIONS = Object.freeze({
-  containerImageVersion: "latest",
-  containerImageName: "cactuts/daml-all-in-one",
+const DEFAULTS = Object.freeze({
+  imageVersion: "2024-09-08T07-40-07-dev-2cc217b7a",
+  imageName: "ghcr.io/hyperledger/cacti-daml-all-in-one",
   rpcApiHttpPort: 7575,
 });
 
+export const DAML_TEST_LEDGER_DEFAULT_OPTIONS = DEFAULTS;
+
 export const DAML_TEST_LEDGER_OPTIONS_JOI_SCHEMA: Joi.Schema =
   Joi.object().keys({
-    containerImageVersion: Joi.string().min(5).required(),
-    containerImageName: Joi.string().min(1).required(),
+    imageVersion: Joi.string().min(5).required(),
+    imageName: Joi.string().min(1).required(),
     rpcApiHttpPort: Joi.number()
       .integer()
       .positive()
@@ -39,8 +41,8 @@ export const DAML_TEST_LEDGER_OPTIONS_JOI_SCHEMA: Joi.Schema =
   });
 
 export class DamlTestLedger implements ITestLedger {
-  public readonly containerImageVersion: string;
-  public readonly containerImageName: string;
+  public readonly imageVersion: string;
+  public readonly imageName: string;
   public readonly rpcApiHttpPort: number;
   public readonly emitContainerLogs: boolean;
 
@@ -48,26 +50,21 @@ export class DamlTestLedger implements ITestLedger {
   private container: Container | undefined;
   private containerId: string | undefined;
 
-  constructor(public readonly options: DAMLTestLedgerConstructorOptions = {}) {
-    if (!options) {
+  constructor(public readonly opts?: IDamlTestLedgerOptions) {
+    if (!opts) {
       throw new TypeError(`DAMLTestLedger#ctor options was falsy.`);
     }
-    this.containerImageVersion =
-      options.containerImageVersion ||
-      DAML_TEST_LEDGER_DEFAULT_OPTIONS.containerImageVersion;
-    this.containerImageName =
-      options.containerImageName ||
-      DAML_TEST_LEDGER_DEFAULT_OPTIONS.containerImageName;
-    this.rpcApiHttpPort =
-      options.rpcApiHttpPort || DAML_TEST_LEDGER_DEFAULT_OPTIONS.rpcApiHttpPort;
-
-    this.emitContainerLogs = Bools.isBooleanStrict(options.emitContainerLogs)
-      ? (options.emitContainerLogs as boolean)
+    this.imageVersion = opts.imageVersion || DEFAULTS.imageVersion;
+    this.imageName = opts.imageName || DEFAULTS.imageName;
+    this.rpcApiHttpPort = opts.rpcApiHttpPort || DEFAULTS.rpcApiHttpPort;
+
+    this.emitContainerLogs = Bools.isBooleanStrict(opts.emitContainerLogs)
+      ? (opts.emitContainerLogs as boolean)
       : true;
 
     this.validateConstructorOptions();
     const label = "daml-test-ledger";
-    const level = options.logLevel || "INFO";
+    const level = opts.logLevel || "INFO";
     this.log = LoggerProvider.getOrCreate({ level, label });
   }
 
@@ -81,7 +78,7 @@ export class DamlTestLedger implements ITestLedger {
   }
 
   public getContainerImageName(): string {
-    return `${this.containerImageName}:${this.containerImageVersion}`;
+    return `${this.imageName}:${this.imageVersion}`;
   }
 
   public async getRpcApiHttpHost(): Promise<string> {
@@ -312,7 +309,7 @@ export class DamlTestLedger implements ITestLedger {
         return NetworkSettings.Networks[networkNames[0]].IPAddress;
       }
     } else {
-      throw new Error(`${fnTag} cannot find image: ${this.containerImageName}`);
+      throw new Error(`${fnTag} cannot find image: ${this.imageName}`);
     }
   }
 
@@ -340,8 +337,8 @@ export class DamlTestLedger implements ITestLedger {
 
   private validateConstructorOptions(): void {
     const validationResult = DAML_TEST_LEDGER_OPTIONS_JOI_SCHEMA.validate({
-      containerImageVersion: this.containerImageVersion,
-      containerImageName: this.containerImageName,
+      imageVersion: this.imageVersion,
+      imageName: this.imageName,
       rpcApiHttpPort: this.rpcApiHttpPort,
     });
 
diff --git a/packages/cactus-test-tooling/src/main/typescript/public-api.ts b/packages/cactus-test-tooling/src/main/typescript/public-api.ts
index d39057572..daba410ae 100755
--- a/packages/cactus-test-tooling/src/main/typescript/public-api.ts
+++ b/packages/cactus-test-tooling/src/main/typescript/public-api.ts
@@ -13,7 +13,11 @@ export {
   IBesuMpTestLedgerOptions,
 } from "./besu/besu-mp-test-ledger";
 
-export { DamlTestLedger } from "./daml/daml-test-ledger";
+export {
+  DamlTestLedger,
+  DAML_TEST_LEDGER_DEFAULT_OPTIONS,
+  IDamlTestLedgerOptions,
+} from "./daml/daml-test-ledger";
 
 export {
   CordaTestLedger,
diff --git a/tools/docker/daml-all-in-one/Dockerfile b/tools/docker/daml-all-in-one/Dockerfile
index d83f3cf80..80195b18a 100644
--- a/tools/docker/daml-all-in-one/Dockerfile
+++ b/tools/docker/daml-all-in-one/Dockerfile
@@ -1,15 +1,17 @@
 FROM ubuntu:22.04
 
-RUN apt update
-RUN apt install curl openjdk-21-jdk -y
+LABEL org.opencontainers.image.source = "https://github.com/hyperledger/cacti"
+
+RUN apt update && \
+    apt install --no-install-recommends curl openjdk-21-jdk supervisor openssl xxd -y
 
 # Download and install DAML SDK 2.9.3
 RUN curl -L https://github.com/digital-asset/daml/releases/download/v2.9.3/daml-sdk-2.9.3-linux.tar.gz | tar -xz -C /opt && \
     cd /opt/sdk-2.9.3 && \
-    ./install.sh
+    ./install.sh && \
+    rm -rf /opt/sdk-2.9.3/
 
 ENV PATH="/root/.daml/bin:${PATH}"
-RUN apt-get install xxd
 RUN daml new quickstart --template quickstart-java
 WORKDIR /quickstart
 
@@ -17,12 +19,10 @@ WORKDIR /quickstart
 RUN echo '{"server": {"address": "0.0.0.0","port": 7575},"ledger-api": {"address": "0.0.0.0","port": 6865}}' > json-api-app.conf
 
 # Run the auto generation of Authorization Bearer Token
-RUN apt-get update && apt-get install -y openssl
 COPY generate-jwt-token.sh /quickstart/generate-jwt-token.sh
 RUN chmod +x /quickstart/generate-jwt-token.sh
 RUN /quickstart/generate-jwt-token.sh
 
-RUN apt-get update && apt-get install -y supervisor
 RUN mkdir -p /var/log/supervisor
 COPY supervisord.conf /etc/supervisord.conf
 

@petermetz
Copy link
Contributor

@raynatopedrajeta
Copy link
Contributor Author

@raynatopedrajeta I'll publish a build of the image from the PR on the official repo shortly. Could you please then update the source code to use that image instead? Also, could you remove the entry "cactuts" from the spell checker configuration? I'm assuming it was a typo while typing "cacti" so we shouldn't add the typo itself in the list of correct words.

Hello @petermetz,
This is noted Peter, we temporarily included "cactuts" on cspell in order for the program to proceed but we are planning to remove this once the image has been created :)

Cheers!
Rayn

@petermetz
Copy link
Contributor

@raynatopedrajeta Got it thank you! I did create and upload the image (link in my previous comment). The suggested edits also contain the source code change that updates the code to use the image I uploaded so if you accept the suggested edits as is then it should be taken care of

@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 3 times, most recently from 836be12 to 28d84cd Compare September 16, 2024 06:57
Copy link
Contributor

@RafaelAPB RafaelAPB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your contribution

@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 2 times, most recently from 0db8b9e to b83dc66 Compare September 18, 2024 07:08
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch from b83dc66 to a3fe238 Compare September 19, 2024 06:58
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raynatopedrajeta We are getting close, thank you for the changes so far!
Please make sure to

  1. Address @RafaelAPB 's earlier request regarding the test cases. Right now if I read it right there is zero coverage for the DamlTestLedger class.
  2. Please update the PR title and the commit message to reflect the nature of the change. It is a feature but not added to the production code and therefore it should have a type of test. Something like: test(test-tooling): add DamlTestLedger implementation is what I would recommend. The reason why this is important is because we are trying to improve the quality of the release notes (the CHANGELOG.md files) which are automatically generated from the commit messages based on their subject line.
  3. Please see: https://github.com/hyperledger/cacti/pull/3462#discussion_r1749101416

@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch 5 times, most recently from f90a485 to 6d5bc90 Compare September 20, 2024 09:49
@raynatopedrajeta raynatopedrajeta changed the title feat(daml): test tooling class test(daml): test tooling Sep 20, 2024
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch from 6d5bc90 to 57992d5 Compare September 20, 2024 10:00
@raynatopedrajeta raynatopedrajeta changed the title test(daml): test tooling test(test tooling): add DamlTestLedger implementation Sep 20, 2024
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch from 57992d5 to 0f9db67 Compare September 20, 2024 10:21
Primary Changes
---------------
1. Create a test tooling class for DAML AIO Image
Fixes hyperledger-cacti#3435

Signed-off-by: raynato.c.pedrajeta <[email protected]>
@raynatopedrajeta raynatopedrajeta force-pushed the raynatopedrajeta/issue3435 branch from 0f9db67 to c6abdd7 Compare September 26, 2024 08:09
@petermetz petermetz merged commit ebe7816 into hyperledger-cacti:main Sep 27, 2024
145 of 148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(daml): test tooling class
4 participants