feat: Add dry run for provisioners by kylecarbs · Pull Request #178 · coder/coder

kylecarbs · 2022-02-07T17:22:04Z

A provisioner can now detect resources that would be provisioned without creating them.

This will be used to enable a preview of resources for a project, and could eventually be used to estimate cost/hour per project.

This was confusing with ParameterValue before. It still is a bit, but this should help distinguish scope.

codecov · 2022-02-07T17:30:50Z

Codecov Report

Merging #178 (0bc1390) into main (427fdc6) will increase coverage by 0.21%.
The diff coverage is 72.38%.

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   67.99%   68.21%   +0.21%     
==========================================
  Files         108      108              
  Lines        5631     5795     +164     
  Branches       68       68              
==========================================
+ Hits         3829     3953     +124     
- Misses       1439     1472      +33     
- Partials      363      370       +7

Flag	Coverage Δ
unittest-go-macos-latest	`66.11% <75.00%> (+0.54%)`	⬆️
unittest-go-ubuntu-latest	`67.55% <71.90%> (+0.34%)`	⬆️
unittest-go-windows-latest	`65.65% <75.80%> (+0.20%)`	⬆️
unittest-js	`64.66% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/projectparameter/projectparameter.go	`68.42% <66.66%> (+0.97%)`	⬆️
provisioner/terraform/provision.go	`71.62% <67.44%> (-4.08%)`	⬇️
provisionerd/provisionerd.go	`69.09% <74.73%> (+1.38%)`	⬆️
coderd/provisionerdaemons.go	`52.18% <100.00%> (+1.19%)`	⬆️
peer/conn.go	`78.97% <0.00%> (-1.03%)`	⬇️
peer/channel.go	`87.19% <0.00%> (+2.43%)`	⬆️
provisioner/echo/serve.go	`50.52% <0.00%> (+3.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 427fdc6...0bc1390. Read the comment docs.

jawnsy · 2022-02-07T19:11:54Z

+			ScopeID: scope.UserID.String,
+		})
+		if err != nil {
+			return nil, err


should this be wrapped?

I think there's a linter that can check for that too.

jawnsy · 2022-02-07T19:13:34Z

 			Name:              parameter.Name,
 			Scope:             database.ParameterScopeWorkspace,
-			ScopeID:           scope.WorkspaceID.String(),
+			ScopeID:           scope.WorkspaceID.UUID.String(),


since ID is typed as a uuid.UUID, why is it that we can't do the same for ScopeID, to avoid the need to call .String?

I wish! We're supporting v1 user and organization IDs, so we have to use string, unfortunately.

Would it be possible to do a one-time migration? Seems unfortunate that we have to support our past decisions indefinitely 😬

I don't see why not. We could probably just add a UUID column to the users and organizations tables in v1, and use them in v2.

jawnsy · 2022-02-07T19:16:23Z

+-- This is to display resources for the started and stopped transitions.
+CREATE TABLE project_version_resource (
+    id uuid NOT NULL UNIQUE,
+    project_version_id uuid NOT NULL REFERENCES project_version (id) ON DELETE CASCADE,


is it possible to use ON DELETE RESTRICT here? it's safer than cascade deletes IMO. cascade deletes can make queries take an unexpectedly long time to run, since it recursively deletes things

Ahh interesting. I suppose that's kinda the best of all worlds. I'll update all of these!

jawnsy · 2022-02-07T19:19:02Z

+    destroy_on_stop boolean NOT NULL,
+    type varchar(256) NOT NULL,
+    name varchar(64) NOT NULL,
+    UNIQUE(project_version_id, type, name)


I think if we add a constraint like this, we should also add an index, otherwise enforcing the constraint will likely require a full table scan

I believe specifying UNIQUE here automatically creates a unique index. It's just a shorthand way to do so.

coadler · 2022-02-07T21:03:48Z

Sort of a drive-by comment, but I'm a bit confused on why we're using varchars everywhere. In Postgres they're pretty much just inferior to the text type.

It's not for performance. Just to ensure a mistake doesn't end up with an attack where someone is inserting 2GB text columns everywhere.

I think this is better caught in the application layer, similar to how we currently require all fields to be validated in the monorepo. Database errors like this are really annoying to bubble up in a user-presentable way, and they require migrations + a full table lock if we ever want to update them.

https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_varchar.28n.29_by_default

Why not both?

Seems kinda like a footgun to me if we could have two (possibly different) arbitrary length limits for each field. If we really want to use the db as a last line of defense we can just have a check constraint for all text fields that limits them to a kib or something large.

kylecarbs added 3 commits February 6, 2022 20:14

refactor: Rename ProjectParameter to ProjectVersionParameter

4345922

This was confusing with ParameterValue before. It still is a bit, but this should help distinguish scope.

Add project version resources table

ac61247

Allow project parameters to optionally have user and workspace

0e7f380

kylecarbs requested a review from bryphe-coder February 7, 2022 17:22

kylecarbs self-assigned this Feb 7, 2022

kylecarbs force-pushed the projectversionresources branch from aaa6801 to d2d9885 Compare February 7, 2022 17:24

Add dry run for provisioners

391640b

kylecarbs force-pushed the projectversionresources branch from d2d9885 to 391640b Compare February 7, 2022 17:34

kylecarbs mentioned this pull request Feb 7, 2022

feat: Add resource detection on project import #181

Closed

Add resource detection on project import

d15f0b3

kylecarbs force-pushed the projectversionresources branch from 3382485 to d15f0b3 Compare February 7, 2022 18:43

kylecarbs force-pushed the projectversionparam branch from 95b1365 to 1af0b50 Compare February 7, 2022 18:45

jawnsy approved these changes Feb 7, 2022

View reviewed changes

coadler reviewed Feb 7, 2022

View reviewed changes

Base automatically changed from projectversionparam to main February 7, 2022 21:40

kylecarbs force-pushed the projectversionresources branch from 3814117 to 22d913d Compare February 7, 2022 21:53

Merge branch 'main' into projectversionresources

c07224e

kylecarbs force-pushed the projectversionresources branch from 22d913d to c07224e Compare February 7, 2022 22:06

kylecarbs added 2 commits February 8, 2022 00:40

Merge branch 'main' into projectversionresources

4f07c3e

Merge branch 'main' into projectversionresources

0bc1390

kylecarbs merged commit 795bba2 into main Feb 8, 2022

kylecarbs deleted the projectversionresources branch February 8, 2022 01:35

Uh oh!

Conversation

kylecarbs commented Feb 7, 2022

Uh oh!

codecov Bot commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coadler Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Feb 7, 2022 •

edited

Loading

kylecarbs Feb 7, 2022 •

edited

Loading

coadler Feb 7, 2022 •

edited

Loading