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

Build match worker image #2313

Merged
merged 47 commits into from
Jan 22, 2024
Merged

Build match worker image #2313

merged 47 commits into from
Jan 22, 2024

Conversation

0xverin
Copy link
Contributor

@0xverin 0xverin commented Dec 6, 2023

Context

resolves p-330

Test steps:

  • Build the worker match parachain version's image and push it to Docker Hub.
  • Docker pull parachain and worker image and ensure it runs successfully.
  • Use the release-ts-api-package gha to generate the client API types.

Copy link

linear bot commented Dec 6, 2023

@0xverin
Copy link
Contributor Author

0xverin commented Dec 7, 2023

After testing, the p0.9.17-9170-w0.0.1-100 release was published, and the worker v0.0.1-100 tag was also pushed, see gha. However, it appears that this worker is still compatible with the parachain latest tag. Testing has shown:

  • worker v0.0.1-100 + parachain v0.9.17 results in failure.

  • worker v0.0.1-100 + parachain latest results in success.

Did I miss anything?

@0xverin 0xverin changed the title [WIP]:Build match worker image Build match worker image Dec 7, 2023
@BillyWooo
Copy link
Collaborator

After testing, the p0.9.17-9170-w0.0.1-100 release was published, and the worker v0.0.1-100 tag was also pushed, see gha. However, it appears that this worker is still compatible with the parachain latest tag. Testing has shown:

  • worker v0.0.1-100 + parachain v0.9.17 results in failure.
  • worker v0.0.1-100 + parachain latest results in success.

Did I miss anything?

Did our session together solve your problem?

@0xverin
Copy link
Contributor Author

0xverin commented Dec 11, 2023

Did our session together solve your problem?

Definitely helpful; it's just that there were some deviations during testing.

@0xverin 0xverin force-pushed the p-330-build-match-worker-image branch from a627359 to 9080e64 Compare December 11, 2023 11:22
@Kailai-Wang
Copy link
Collaborator

Kailai-Wang commented Dec 11, 2023

Is this PR ready to be reviewed?

I see there're unfinished TODOs but at the same time WIP was removed from the title and this is not a draft PR anymore

@Traf333 Traf333 force-pushed the p-330-build-match-worker-image branch from 2d26ebb to 8924412 Compare January 17, 2024 15:59
Copy link
Contributor Author

@0xverin 0xverin left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. Thank you for resolving the connect issue.

default: 'latest'
inputs:
release-tag:
description: "Client-api release tag (e.g. p1.2.0-9701-w0.0.1-101)"
Copy link
Contributor Author

@0xverin 0xverin Jan 19, 2024

Choose a reason for hiding this comment

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

We can't use the latest version if we change it here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

it won't affect the use of lates version and we still are able to set release tag as latest in order to generate types from latest docker images

@@ -13,11 +13,13 @@ services:
litentry-node:
condition: service_healthy
litentry-worker-1:
condition: service_healthy
# using +service_started+ over +service_healthy+ since worker runs successfully but can not connect to parachain
Copy link
Contributor Author

Choose a reason for hiding this comment

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

3

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "3" mean?

@Traf333 Traf333 enabled auto-merge (squash) January 22, 2024 09:17
@Traf333 Traf333 merged commit 3869810 into dev Jan 22, 2024
31 checks passed
@Traf333 Traf333 deleted the p-330-build-match-worker-image branch January 22, 2024 09:22
Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

Sorry I'm a bit late to it - just found it was merged already

No worries, we can always make it up with subsequent changes.

@@ -114,8 +115,8 @@ jobs:
${{ matrix.chain }}-parachain-srtool-digest.json
${{ matrix.chain }}-parachain-runtime.compact.compressed.wasm

## build docker image of parachain binary ##
build-docker:
# build docker image of parachain binary ##
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to change it to single #? It used the ## ... ## style

@@ -127,7 +128,7 @@ jobs:

- name: Set env
run: |
DOCKER_TAG=$(echo ${{ env.RELEASE_TAG }} | cut -d'-' -f1 | sed 's/p/v/')
DOCKER_TAG=$(echo ${{ env.RELEASE_TAG }} | sed 's/p/v/;s/\(.*\)-w.*/\1/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason for this, or just a casual change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we change the job name from build-docker to build-parachain-docker, we should change the var to PARACHAIN_DOCKER_TAG too, for workers it should be WORKER_DOCKER_TAG - please let's keep the naming consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for this, or just a casual change?
Yes this changes allows us to consider a variety of options for specific versions

  • latest
  • p0.9.18-w0.0.2
  • p0.9.18-9181-w0.0.2-101

Comment on lines +184 to +201
- name: Free up disk space
if: startsWith(runner.name, 'GitHub Actions')
uses: jlumbroso/free-disk-space@main
with:
tool-cache: true
swap-storage: false
large-packages: false

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
with:
# use the docker driver to access the local image
# we don't need external caches or multi platforms here
# see https://docs.docker.com/build/drivers/
driver: docker

- name: Cache worker-cache
uses: actions/cache@v3
Copy link
Collaborator

Choose a reason for hiding this comment

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

For releases I suggest not using any cache at all

networks:
- litentry-test-network
entrypoint:
"/usr/local/worker-cli/lit_ts_api_package_build.sh -p 9912 -u ws://litentry-node
entrypoint: "/usr/local/worker-cli/lit_ts_api_package_build.sh -p 9912 -u ws://litentry-node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we keep the old format?

@@ -13,11 +13,13 @@ services:
litentry-node:
condition: service_healthy
litentry-worker-1:
condition: service_healthy
# using +service_started+ over +service_healthy+ since worker runs successfully but can not connect to parachain
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "3" mean?

# using +service_started+ over +service_healthy+ since worker runs successfully but can not connect to parachain
# as requires additional pre-setup for parachain image which built in production mode
# for generating types there is no need for fully workable interaction between worker and parachain
condition: service_started
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mean we can get the data even tho enclave is not registered on the parachain? (I assume so

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's correct. I have one concern though, what if not registred enclave generate types which are different in case it was registred 🤔

@Traf333
Copy link
Contributor

Traf333 commented Jan 22, 2024

Sorry I'm a bit late to it - just found it was merged already

No worries, we can always make it up with subsequent changes.

Opening follow up PR for that. I've also observed one critical part which was commented out for testing purpose nope all seems fine, just cached diff

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.

None yet

4 participants