-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add tests for external dependencies in GCP #10775
Conversation
c608587
to
95748d8
Compare
5cba389
to
e95fc6f
Compare
95748d8
to
c6ea65b
Compare
3829dec
to
0ade299
Compare
#10688 has been merged, this is ready to be rebased against main. 😄 |
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 see some potential for risk in how we're selecting randomized test cases but we should be able to merge this in time for the end of Q2. Will review this again after the rebase but I don't see any blockers.
database_version = "MYSQL_5_7" | ||
region = "${var.region}" | ||
settings { | ||
tier = "db-n1-standard-2" |
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.
Question: Do we have a good understanding of our database load under normal operating conditions and test condition? Given that we're not going to be hitting the database that hard can we opt for a cheaper DB tier and increase it if testing scenarios demand more memory/throughput?
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.
This is a good point, ideally we can make this configurable and use a cheaper one for tests. I was basically just translating the reference architecture here. I was thinking that since our test setup is ephemeral the extra cost we pay here is quite small. May be I am wrong, and it adds up. I will make a note for this for a follow-up
.werft/installer-tests.ts
Outdated
@@ -41,14 +116,22 @@ const INFRA_PHASES: { [name: string]: InfraConfig } = { | |||
makeTarget: "managed-dns", | |||
description: "Sets up external-dns & cloudDNS config", | |||
}, | |||
GENERATE_KOTS_CONFIG: { | |||
phase: "generate-kots-config", | |||
makeTarget: `generate-kots-config storage=${randomize("storage", cloud)} registry=${randomize( |
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.
Suggestion: performing randomization in this location means that loading this file isn't an idempotent operation. If a randomized test configuration fails we'll likely need to repeat that test case (either to rule out a transient failure or reproduce the failure). Because these settings are encoded during the file evaluation we don't have a means of injecting an exact configuration.
Phase parameterization can be kept out of scope for this PR (espcially because it'll take time to define how we configure phases), but we should definitely follow up on this in another issue/PR for Q3. If we defer to Q3 I'd love to capture this requirement in a ticket before we merge this PR.
EDIT: Leaving the above comment as it's still relevant, but less so.
In install/tests/Makefile
we do provide mechanisms for setting the exact configuration so we should be able to reproduce a given test case if we omit werft. This reduces the challenge of randomizing these values 🎉 , but we'll still need to account for this in the future.
install/tests/Makefile
Outdated
@echo | ||
@echo "URL of your setup is: "https://$(TF_VAR_TEST_ID).gitpod-self-hosted.com"" | ||
@echo | ||
@echo "The IP address of you setup is "https://$(TF_VAR_TEST_ID).gitpod-self-hosted.com"" |
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.
Note: will this subdomain need to change given the issues we've had with domain level wildcard letsencrypt issuance?
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 will highly likely stick with the same subdomain, just we will try and re-use the certs issued across multiple sub-domains(max 200 subdomains can be added under a certificate, iirc). In reality, I don't think we will hit a limit even if we create new certs per domain. This week it so happened because I was running too many tests on my own
0ade299
to
5a79d8b
Compare
5a79d8b
to
a3d4434
Compare
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!
Description
This PR adds the external dependencies test for GCP cloud platform. These dependencies include
cloudsql
as external database,GCR
as external registry, andcloud storage
as external storage. These dependencies are randomly selected for each test providing us a overall testing of every combination over a course of days.Related Issue(s)
Fixes #
How to test
You can run the following command to test one of the available combinations:
Release Notes
Documentation
Werft options: