Skip to content
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

e2e test of managed service deployment #873

Merged

Conversation

BROngineer
Copy link
Contributor

@BROngineer BROngineer commented Jan 8, 2025

  • Add test of deploying service using ServiceTemplate included into ClusterDeployment

Fixes: #849 #850

@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch 2 times, most recently from 6eb2fed to 4dcd7e7 Compare January 9, 2025 00:35
Copy link
Contributor

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM, a couple of questions/comments, and the PR should be postponed until the e2e CI is ready (as of now and to my understanding it is blocked due to the #871). After addressing the issue, the PR should be then checked with the e2e tests job

test/e2e/provider_aws_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@eromanova eromanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. One small question:
Should we also test service deployment for other providers or hosted environments? I’m asking because once #752 is implemented, some providers might be skipped, and service deployment may not be checked at all. But, if this code is expected to change soon, it’s fine as is for now.

test/e2e/clusterdeployment/servicevalidator.go Outdated Show resolved Hide resolved
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch 3 times, most recently from 1b50d68 to 0cdf5ca Compare January 19, 2025 22:21
@BROngineer BROngineer added the test e2e Runs the entire provider E2E test suite, controller E2E tests are always ran label Jan 19, 2025
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from 3b1d949 to d3b263f Compare January 20, 2025 08:24
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from d3b263f to 9a8bfd0 Compare January 20, 2025 12:32
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from e4df9c0 to b5bd1d6 Compare January 20, 2025 12:42
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from b5bd1d6 to 10e370c Compare January 20, 2025 13:51
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from 10e370c to f04cc90 Compare January 20, 2025 13:53
zerospiel
zerospiel previously approved these changes Jan 20, 2025
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from f04cc90 to 9fcbfcb Compare January 20, 2025 19:22
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from 9fcbfcb to c0ade0b Compare January 21, 2025 07:49
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from c0ade0b to f98e341 Compare January 21, 2025 08:19
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from f98e341 to 6db508b Compare January 21, 2025 11:20
@BROngineer
Copy link
Contributor Author

@zerospiel @eromanova @Kshatrix colleagues, e2e tests are flacky. What's especially annoying is that their stability depends on the cloud provider. GH actions history shows, that tests added in this PR passes in case kcm able to deploy the cluster. Pls approve and merge.

@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from 6db508b to 4551fcf Compare January 22, 2025 08:15
Signed-off-by: Artem Bortnikov <[email protected]>
Signed-off-by: Artem Bortnikov <[email protected]>
Signed-off-by: Artem Bortnikov <[email protected]>
Signed-off-by: Artem Bortnikov <[email protected]>
Signed-off-by: Artem Bortnikov <[email protected]>
Signed-off-by: Artem Bortnikov <[email protected]>
@BROngineer BROngineer force-pushed the epic-829/deploy-services-with-servicetemplate branch from 4551fcf to 23473ad Compare January 22, 2025 19:19
Copy link
Collaborator

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed previously since tests are failing randomly (in other PRs as well) we can't wait indefinitely for the tests to pass.
Merging without green tests. Code itself looks good, but it must be revisited after service templates will be separated to the catalog repo

@a13x5 a13x5 merged commit dd905e3 into k0rdent:main Jan 22, 2025
1 of 3 checks passed
@BROngineer BROngineer deleted the epic-829/deploy-services-with-servicetemplate branch January 22, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test e2e Runs the entire provider E2E test suite, controller E2E tests are always ran
Projects
Status: Done
4 participants