-
Notifications
You must be signed in to change notification settings - Fork 37
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
Initial E2E Tests #387
base: main
Are you sure you want to change the base?
Initial E2E Tests #387
Conversation
Contributed on behalf of STMicroelectronics
This will through an error otherwise
strategy: | ||
fail-fast: false | ||
matrix: | ||
kubernetes: [v1.32.0, v1.31.4, v1.30.8, v1.29.12] |
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'm not sure how many versions we should include here. Kubernetes typically provides patches for the last 3-4 releases. I think we should at least test the latest version and the oldest one within this range.
It might also be useful to include all versions, especially when updating our Kubernetes client API libraries. In such cases, if something breaks, it would be helpful to pinpoint exactly which versions have the issue.
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 makes sense to include all versions that are not end of life. Thus, I'd just leave it like this :)
6d54450
to
27e2119
Compare
27e2119
to
c4232a9
Compare
This reverts commit d86d7d6.
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.
Hi @jfaltermeier , Thanks for providing this vast improvement in testing 🥳
The changes already look pretty good to me. I only have a few small remarks inline.
path: "./theia-cloud-helm" | ||
|
||
- name: Setup Minikube | ||
uses: manusa/actions-setup-minikube@92af4db914ab207f837251cd53eb7060e6477614 |
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.
Minor: add comment which version this commit hash resolves to.
|
||
- name: Enable Minikube Addons | ||
run: | | ||
minikube addons enable dashboard |
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.
What is the dashboard needed for in E2E tests?
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 the file name contains a typo and the file should be named constants.ts
?
|
||
name = "theia-cloud-crds" | ||
chart = "../../../theia-cloud-helm/charts/theia-cloud-crds" | ||
namespace = "theiacloud" |
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.
Small detail: AFAIK since we aligned all namings, our default namespace name is theia-cloud
. Should we adapt this here for the sake of consistency?
If yes, also adapt below
kubectl -n ingress-nginx delete pod -l app.kubernetes.io/name=ingress-nginx | ||
``` | ||
|
||
Adapt your environment so that all docker images are built in minikube. Build all Theia Cloud docker images + Demos with tag `minikube-ci-e2e`, e.g. `theiacloud/theia-cloud-service:minikube-ci-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.
We should add a note that the docker build
commands need to be executed in the repository root to avoid confusioon
strategy: | ||
fail-fast: false | ||
matrix: | ||
kubernetes: [v1.32.0, v1.31.4, v1.30.8, v1.29.12] |
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 makes sense to include all versions that are not end of life. Thus, I'd just leave it like this :)
Use squash on merge
This PR establishes a foundation for implementing E2E tests.
It has a matrix GitHub workflow that combines different configurations: Kubernetes versions, ephemeral vs. persistent storage, subdomain vs. path-based routing, and Keycloak enablement.
Currently, the tests are just checking login/logout and starting a session, but this provides a base that can be extended with additional tests.
It also fixes an issue with the
MinikubePersistentVolumeCreator
that caused an error.Test Run: https://github.com/eclipse-theia/theia-cloud/actions/runs/12413770653
Contributed on behalf of STMicroelectronics