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

Introduce running of protocol substrate node and dkg node via the test-utils package #278

Merged
merged 33 commits into from
Sep 29, 2022

Conversation

dharjeezy
Copy link
Contributor

@dharjeezy dharjeezy commented Sep 6, 2022

PR Description

This PR solves part of the issue in #270 so as to start up different node types such as

  1. The Protocol substrate node
  2. The DKG node

} from './substrateNodeBase.js';

const STANDALONE_DOCKER_IMAGE_URL =
'ghcr.io/webb-tools/protocol-substrate-standalone-node:edge';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use stable version here?

Copy link
Contributor Author

@dharjeezy dharjeezy Sep 7, 2022

Choose a reason for hiding this comment

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

The stable version is not with consistent with master.

Copy link
Contributor

@drewstone drewstone Sep 7, 2022

Choose a reason for hiding this comment

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

Thanks for checking @dharjeezy, @dutterbutter please make sure we keep things up to date. I'm not sure what stable is but possibly worth removing. Would rather we consider something called production instead of stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

stable is a deployed docker image on protocol-substrate. It's purpose is to prevent all of our CIs from breaking when a breaking change is made on protocol-substrate. Changing the name is trivial - but would welcome suggestions on better methods for releasing software incrementally across our repos.

image: string;
}): void {
if (!this.checkIfImageExists(opts.image) || opts.frocePull) {
execSync(`docker pull ${opts.image}`, {

Check warning

Code scanning / CodeQL

Unsafe shell command constructed from library input

[String concatenation](1) based on [library input](2) is later used in [shell command](3).
@dharjeezy dharjeezy changed the title substrate package util Introduce running of protocol substrate node and dkg node via the test-utils package Sep 6, 2022
@dharjeezy dharjeezy marked this pull request as ready for review September 7, 2022 13:04
Copy link
Contributor

@drewstone drewstone left a comment

Choose a reason for hiding this comment

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

LG so far, need more comments and would appreciate formatting more things.

tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
"main": "./cjs/index.js",
"module": "./index.js",
"types": "./index.d.ts",
"main": "./build/cjs/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @nepoche to check these since he would know best if this is how we should point to the files. Have a feeling we shouldn't have this. Any reason why its added @dharjeezy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yh, so i was having issue trying to use the package in the integration tests. So, i had to point to the files like that just as it is being done here https://github.com/webb-tools/webb.js/blob/master/packages/sdk-core/package.json

Copy link
Contributor

Choose a reason for hiding this comment

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

This is consistent with the way we are building packages for the monorepo, esm, and cjs.

packages/test-utils/src/substrate/substrateNodeBase.ts Outdated Show resolved Hide resolved
@dutterbutter dutterbutter added the needs review 👓 Pull request needs a review label Sep 7, 2022
.eslintignore Outdated Show resolved Hide resolved
"main": "./cjs/index.js",
"module": "./index.js",
"types": "./index.d.ts",
"main": "./build/cjs/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is consistent with the way we are building packages for the monorepo, esm, and cjs.

packages/test-utils/src/substrate/substrateNodeBase.ts Outdated Show resolved Hide resolved
packages/test-utils/src/substrate/substrateNodeBase.ts Outdated Show resolved Hide resolved
packages/test-utils/src/substrate/substrateNodeBase.ts Outdated Show resolved Hide resolved
} from './substrateNodeBase.js';

const STANDALONE_DOCKER_IMAGE_URL =
'ghcr.io/webb-tools/protocol-substrate-standalone-node:edge';
Copy link
Contributor

Choose a reason for hiding this comment

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

stable is a deployed docker image on protocol-substrate. It's purpose is to prevent all of our CIs from breaking when a breaking change is made on protocol-substrate. Changing the name is trivial - but would welcome suggestions on better methods for releasing software incrementally across our repos.

tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
@dutterbutter dutterbutter added needs changes 🔧 PR has requested changes or conflicts that need to be addressed and removed needs review 👓 Pull request needs a review labels Sep 8, 2022
@dharjeezy dharjeezy added needs review 👓 Pull request needs a review and removed needs changes 🔧 PR has requested changes or conflicts that need to be addressed labels Sep 8, 2022
@dutterbutter dutterbutter requested review from nepoche and removed request for dutterbutter September 14, 2022 08:25
tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
tests/utils/backend-utils.ts Outdated Show resolved Hide resolved
@nepoche nepoche added needs changes 🔧 PR has requested changes or conflicts that need to be addressed and removed needs review 👓 Pull request needs a review labels Sep 15, 2022
@dutterbutter
Copy link
Contributor

@dharjeezy please resolve conflicts

@dutterbutter dutterbutter removed their request for review September 28, 2022 13:25
…ami/consolidate-test-util

� Conflicts:
�	packages/test-utils/package.json
@drewstone drewstone merged commit dfb0bc0 into master Sep 29, 2022
@drewstone drewstone deleted the dami/consolidate-test-util branch September 29, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changes 🔧 PR has requested changes or conflicts that need to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants