-
Notifications
You must be signed in to change notification settings - Fork 1k
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
azure e2e test #1241
azure e2e test #1241
Conversation
RUN apt-get install -y kubectl | ||
|
||
# Install helm |
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.
not sure if the feast-ci image already has kubectl and/or helm when it is running tests, so it's possible these installs are not needed. Also lmk if there's a better place to install the azure CLI
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 its fine, but should the version be pinned?
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.
the version for which of these?
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'd pin both kubectl and helm. kubectl is only compatible within one minor version so it is conceivable that one day this will suddenly stop working with our k8s clusters.
echo "${STEP_BREADCRUMB} k8s sanity check" | ||
kubectl get pods | ||
|
||
# e2e test - runs in sparkop namespace for consistency with AWS sparkop test. |
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.
Is there a reason to keep these consistent? No conflict will occur?
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 just so I can reuse the existing sparkop test code as is, and it needs to run in some namespace so why not "sparkop"
infra/scripts/azure-runner.sh
Outdated
echo "########## Starting e2e tests for ${GIT_REMOTE_URL} ${GIT_TAG} ###########" | ||
|
||
# This seems to make builds a bit faster. | ||
export DOCKER_BUILDKIT=1 |
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.
don't need this if not building any docker images
infra/scripts/azure-runner.sh
Outdated
- kind: ServiceAccount | ||
name: default | ||
EOF | ||
} |
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 may make sense to put these functions in a separate file since they're used by the AWS test scripts as well
Signed-off-by: Jacob Klegar <[email protected]>
Signed-off-by: Jacob Klegar <[email protected]>
Signed-off-by: Jacob Klegar <[email protected]>
e3ae129
to
6fc77c4
Compare
/retest |
@jklegar: The following test 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jklegar, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Jacob Klegar [email protected]
What this PR does / why we need it: Runs e2e tests on Azure using the new spark operator and Azure blob storage support. Note it therefore requires #1218
Which issue(s) this PR fixes:
Fixes #1181
Does this PR introduce a user-facing change?: