feat: Add dry run for provisioners #178
Conversation
This was confusing with ParameterValue before. It still is a bit, but this should help distinguish scope.
kylecarbs
force-pushed
the
projectversionresources
branch
from
aaa6801
to
d2d9885
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
kylecarbs
force-pushed
the
projectversionresources
branch
from
d2d9885
to
391640b
Compare
kylecarbs
force-pushed
the
projectversionresources
branch
from
3382485
to
d15f0b3
Compare
kylecarbs
force-pushed
the
projectversionparam
branch
from
95b1365
to
1af0b50
Compare
| ScopeID: scope.UserID.String, | ||
| }) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
should this be wrapped?
I think there's a linter that can check for that too.
| @@ -163,7 +170,7 @@ func TestCompute(t *testing.T) { | |||
| ID: uuid.New(), | |||
| Name: parameter.Name, | |||
| Scope: database.ParameterScopeWorkspace, | |||
| ScopeID: scope.WorkspaceID.String(), | |||
| ScopeID: scope.WorkspaceID.UUID.String(), | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I wish! We're supporting v1 user and organization IDs, so we have to use string, unfortunately.
There was a problem hiding this comment.
Would it be possible to do a one-time migration? Seems unfortunate that we have to support our past decisions indefinitely 😬
There was a problem hiding this comment.
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.
| CREATE TABLE project_version_resource ( | ||
| id uuid NOT NULL UNIQUE, | ||
| project_version_id uuid NOT NULL REFERENCES project_version (id) ON DELETE CASCADE, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ahh interesting. I suppose that's kinda the best of all worlds. I'll update all of these!
| type varchar(256) NOT NULL, | ||
| name varchar(64) NOT NULL, | ||
| UNIQUE(project_version_id, type, name) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I believe specifying UNIQUE here automatically creates a unique index. It's just a shorthand way to do so.
| destroy_on_stop boolean NOT NULL, | ||
| type varchar(256) NOT NULL, | ||
| name varchar(64) NOT NULL, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
force-pushed
the
projectversionresources
branch
from
3814117
to
22d913d
Compare
kylecarbs
force-pushed
the
projectversionresources
branch
from
22d913d
to
c07224e
Compare
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.