-
Notifications
You must be signed in to change notification settings - Fork 874
feat: persist prebuild definitions on template import #16951
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
Conversation
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
appeasing linter Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.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.
Self Review notes
70ecdcc
to
7e062e2
Compare
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
appeasing linter Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
7e062e2
to
51773ec
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.
@SasSwart Are you sure about the scope of the PR? I see a lot of changes pulled from mainline.
InvalidateAfterSecs: sql.NullInt32{ | ||
Int32: 0, | ||
Valid: false, | ||
}, // TODO: implement cache invalidation |
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.
Re TODO: perhaps create an issue for that and link it here
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.
👍
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.
Post-merge review
@@ -182,7 +182,6 @@ func TestDBAuthzRecursive(t *testing.T) { | |||
method.Name == "PGLocks" { | |||
continue | |||
} | |||
// Log the name of the last method, so if there is a panic, it is |
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.
Intentional deletion?
@@ -883,10 +884,24 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s | |||
) | |||
} | |||
|
|||
if len(preset.Prebuilds) != 1 { |
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.
Will this not warn incorrectly when there are no prebuilds defined at all (which is perfectly valid)?
Shouldn't this be len(preset.Prebuilds) > 1
?
@@ -2,7 +2,7 @@ terraform { | |||
required_providers { | |||
coder = { | |||
source = "coder/coder" | |||
version = "2.1.3" | |||
version = "2.3.0-pre2" |
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.
Now that coder/terraform-provider-coder#373 is merged we should cut a release and reference it here.
This PR allows provisioners to recognise and report prebuild definitions to the coder control plane. It also allows the coder control plane to then persist these to its store.
closes coder/internal#507