-
Notifications
You must be signed in to change notification settings - Fork 250
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
[QE] e2e oci image refactor #4065
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bb0c5f1
to
11d58b7
Compare
4638abf
to
43f18cc
Compare
1395753
to
d87af6a
Compare
579b9dd
to
5e3e0bf
Compare
-e TARGET_HOST=XXXX \ | ||
-e TARGET_HOST_USERNAME=XXXX \ | ||
-e TARGET_HOST_KEY_PATH=/data/id_rsa \ | ||
-e TARGET_FOLDER=crc-e2e \ |
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.
Does this TARGET_FOLDER
override the ASSETS_FOLDER
of the image? because as part of container file following is what we mention
COPY images/build-e2e/lib/${OS}/* ${ASSETS_FOLDER}/
and during the build of this container image from makefile I don't see passing a ASSETS_FOLDER so not sure how run.ps1/run.sh
lands to this TARGET_FOLDER
.
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, TARGET_FOLDER takes effect on run time, and it makes reference to the folder path where we want to copy the required resources on the target host
The ASSETS_FOLDER is defined on build time, and it is being used by the base image quay.io/rhqp/deliverest:v0.0.5
Deliverest image has some common bash functions to ssh / scp and implement the logic:
- Copy everything on the container
ASSETS_FOLDER
to the remote targetTARGET_FOLDER
- Run on the remote target the command of the container
- Copy back the folder from the target host at
TARGET_RESULTS
to the container path defined byOUTPUT_FOLDER
- Remove the
TARGET_FOLDER
on the target host
This logic is being used on almost every test execution done against a remote target, this is why it has been abstracted into Deliverest and now each image inheriting from it just need to define its specific workload per platform / arch
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.
Okay so this is part of https://github.com/adrianriobo/deliverest/blob/main/entrypoint.sh#L37 one.
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 exactly, at some point I need to move those small projects to some org
@adrianriobo Because of this PR looks like all the openshift CI tests are failing. https://github.com/crc-org/crc/blob/main/images/openshift-ci/Dockerfile#L12-L13 ? The path in the makefile was |
8ba0a48
to
123302a
Compare
Now it should be good I refactored a bit the e2e and integration building blocks on the Makefile to accommodate the changes |
Wait for my ack on test to have another look I will let you know |
Signed-off-by: Adrian Riobo Lorenzo <[email protected]>
…arallel Signed-off-by: Adrian Riobo Lorenzo <[email protected]>
@adrianriobo: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@praveenkumar it is working now you can review if you want the latest changes |
Fixes #4040
Fixes #3932
Depends on #4019