chore: enable terraform provisioners in e2e by default#13134
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
da51c9b to
487f0a8
Compare
Opt out supported for "lite" tests. Environment checking happens to verify terraform + docker exist before running.
| "typescript.tsdk": "./site/node_modules/typescript/lib" | ||
| "typescript.tsdk": "./site/node_modules/typescript/lib", | ||
| // Playwright tests in VSCode will open a browser to live "view" the test. | ||
| "playwright.reuseBrowser": true |
There was a problem hiding this comment.
This is required to run a "live" debugger in VSCode with the chrome window open the whole time. Otherwise the window keeps opening and closing for each snapshot step.
| // StarterTemplates are ids of starter templates that can be used in place of | ||
| // the responses payload. These starter templates will require real provisioners. | ||
| export enum StarterTemplates { | ||
| STARTER_DOCKER = "docker", | ||
| } |
There was a problem hiding this comment.
We should probably add ways to add more to the templates later. For now, just using the starter template is easier.
| // Disabling terraform tests is optional for environments without Docker + Terraform. | ||
| // By default, we opt into these tests. | ||
| export const requireTerraformTests = !process.env.CODER_E2E_DISABLE_TERRAFORM; |
There was a problem hiding this comment.
I want to include this option if the Terraform stuff ever gets a bit messy. Right now for example docker containers are leaky.
| test.skip( | ||
| true, | ||
| "creating docker containers is currently leaky. They are not cleaned up when the tests are over.", | ||
| ); |
There was a problem hiding this comment.
I want to resolve this in another PR. Terraform provisioners are now working, just need to do some cleanup before I allow this test into the mainstage.
| // Will assume the query param has a valid ProvisionerType, as this query param is only used | ||
| // in testing. | ||
| defaultInitialValues.provisioner_type = | ||
| (searchParams.get("provisioner_type") as ProvisionerType) || "terraform"; |
There was a problem hiding this comment.
I'd kind of rather leave this as a string, because we're not doing any validation on it, so it could be anything
There was a problem hiding this comment.
You mean make provisioner_type a string?
There was a problem hiding this comment.
I think the .get here returns string | undefined, and the || narrows it to string, but I think the cast here narrow it more than it should, basically. we don't actually know that the returned value is a valid ProvisionerType here because we didn't check!
There was a problem hiding this comment.
Yes, that is correct. The type of the defaultInitialValues.provisioner_type is ProvisionerType which I would like to keep. Essentially I think adding validation is probably the best approach, but since this is a hidden query param only used in testing, I just skipped it for now. The BE will return a validation error if someone discovers this and tries to do something unsupported.
aslilac
left a comment
There was a problem hiding this comment.
thanks for addressing things! nothing worth blocking over anymore
Opt out supported for "lite" tests. Environment checking happens
to verify terraform + docker exist before running.
The error if you are missing
docker(orterraform):Closes #12821