-
Notifications
You must be signed in to change notification settings - Fork 19
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
P 1139 omniexecutor ci and deployment for dev #3181
P 1139 omniexecutor ci and deployment for dev #3181
Conversation
BillyWooo
commented
Nov 19, 2024
- create litentry/omni-executor docker image
- add skeleton for CI test
- update Makefile, move executable and manifest with same folder level, as well as signature file.
- use Alice as default sign account. Will update this later with predefined account; or randomly generated account for testing purpose.
…-and-deployment-for-dev
…e switch to `loader.entrypoint.uri = "file:..."`
…-and-deployment-for-dev
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.
In general it looks good
.github/workflows/ci.yml
Outdated
@@ -285,7 +293,7 @@ jobs: | |||
working-directory: ./tee-worker | |||
shell: bash | |||
run: | | |||
for d in . identity/enclave-runtime bitacross/enclave-runtime; do | |||
for d in . identity identity/enclave-runtime; do |
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.
Hmm why this change?
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.
previously tee-check
checks enclave-runtime
from both identity
and bitacross
. But didn't check other files within identity
and bitacross
.
I changed it to two jobs, each of them check files both within enclave-runtime
and outside.
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 think it does.
for d in . identity/enclave-runtime bitacross/enclave-runtime; do
does the workspace-wide check as they belong to the same workspace
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. You are right they are in the same workspace. I reverted the changes.
.github/workflows/ci.yml
Outdated
# see https://docs.docker.com/build/drivers/ | ||
driver: docker | ||
|
||
- name: Build local builder |
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.
Maybe we don't need this intermediate image while still having two stages in Dockerfile
It's built for other two workers for cache purpose
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 think we still need two stages. Actually I tried to merge them into one step. But the builder
stage uses rust:1.82-bookworm
, which is quite big. The final image pushed is also very big. That's the reason I revert back to two stages.
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 still have two stages in Dockerfile, but we don't need to build a local-builder
first IMO
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 have removed the local-builder
.
.github/workflows/ci.yml
Outdated
timeout-minutes: 40 | ||
run: | | ||
docker compose -f docker-compose.yml | ||
# docker compose -f docker-compose.yml -f ${{ matrix.test_name }}.yml up --no-build --exit-code-from ${{ matrix.test_name }} ${{ matrix.test_name }} |
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 remove commented lines
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.
😂 These lines I left there on purpose. The above line is just placeholder. So it can easily to extend in the future.
45, 219, 105, 155, 49, 74, 164, 131, 153, 192, 15, 213, 225, 179, 167, 129, 12, 160, | ||
229, 37, 133, 168, 141, 233, 98, 117, 254, 112, 139, 210, 76, 6, | ||
229, 190, 154, 80, 146, 184, 27, 202, 100, 190, 129, 210, 18, 231, 242, 249, 235, 161, | ||
131, 187, 122, 144, 149, 79, 123, 118, 54, 31, 110, 219, 92, 10, |
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.
It's better to use explict expression than magic numbers
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. I will improve it.
f54d5ec
to
b34749c
Compare
tested on tee-dev. |