- Notifications
You must be signed in to change notification settings - Fork928
chore: enable terraform provisioners in e2e by default#13134
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedMay 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
da51c9b to487f0a8CompareOpt out supported for "lite" tests. Environment checking happensto 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should probably add ways to add more to the templates later. For now, just using the starter template is easier.
Uh oh!
There was an error while loading.Please reload this page.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
Tracked here:#13163
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd kind of rather leave this as astring, because we're not doing any validation on it, so it could be anything
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.
You mean makeprovisioner_type a string?
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.get here returnsstring | undefined, and the|| narrows it tostring, but I think the cast here narrow it more than it should, basically. we don't actually know that the returned value is a validProvisionerType here because we didn't check!
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.
Yes, that is correct. The type of thedefaultInitialValues.provisioner_type isProvisionerType 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
thanks for addressing things! nothing worth blocking over anymore
Uh oh!
There was an error while loading.Please reload this page.
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