-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added e2e tests prcheck workflow #236
Conversation
19a6d6c
to
fe55658
Compare
.github/workflows/pr-check.yaml
Outdated
path: podman-desktop | ||
|
||
- name: Update podman |
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.
No need now. please remove
.github/workflows/pr-check.yaml
Outdated
podman build -t local_sandbox_image ./ | ||
CONTAINER_ID=$(podman create localhost/local_sandbox_image --entrypoint "") | ||
podman export $CONTAINER_ID > /tmp/local_sandbox_image.tar |
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.
please update according to ai-lab changes.
Signed-off-by: xbabalov <[email protected]>
Signed-off-by: xbabalov <[email protected]>
Signed-off-by: xbabalov <[email protected]>
This issue prevents the tests from passing #245. Tests pass if we delete the resources folder beforehand. |
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.
LGTM. Code is working fine, tested locally. although we have a issue on the PR check itself when building extension locally.
@dgolovin Do you suggest we merge this PR failing on this issue #245 or do I delete the needed folder, we merge it passing and when the issue is fixed the tests fail and I fix additionally after that if needed? The passing tests would help when trying to test other PRs later but if the blocker is planned to get fixed soon we can just merge it? |
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.
LGTM
Issue: #200