- Notifications
You must be signed in to change notification settings - Fork907
chore: add dynamic parameter warning if missing metadata#17809
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
Dynamic parameters require the latest provisioner to function
func NewServer(lifecycleCtx context.Context, | ||
func NewServer( | ||
lifecycleCtx context.Context, | ||
apiVersion 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 don't feel like coderd should be telling the provisioner what it's version is. This would break on deployments with external provisioners that are on an older version. and I know that's not really a thing we prioritize supporting, but this masks over the mismatched versions in a bad way.
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.
Coderd does not tell the provisioner.
External provisioners call coderd viaprovisionerDaemonServe
In that, they add a query param of their version:
coder/enterprise/coderd/provisionerdaemons.go
Lines 252 to 255 ineb656ef
apiVersion:="1.0" | |
ifqv:=r.URL.Query().Get("version");qv!="" { | |
apiVersion=qv | |
} |
Which is saved to the DB, and what we show on the provisioners page:
APIVersion:apiVersion, |
And in that serve function, that apiVersion is sent to the go routine that manages the gRPC connection:
daemon.APIVersion, |
So theapiVersion
in thatNewServer
is the Coderd side of the provisioner. And that version is sourced from the external provisionerd
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.
The daemon client sends the version here:
coder/codersdk/provisionerdaemons.go
Line 273 ineb656ef
query.Add("version",proto.CurrentVersion.String()) |
Idk why it sets it twice
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.
func parameterProvisionerVersionDiagnostic(tf database.TemplateVersionTerraformValue) hcl.Diagnostics { | ||
missingMetaData := hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "This template version is missing required metadata to support dynamic parameters. Go back to the classic creation flow.", |
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.
hmm. I don't like this. I would much prefer if we could have a way to expose this naturally to the frontend, forcing it over to the classic form. why make everyone click the "dynamic parameters suck" button? that's a super bad first impression
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 are right, it is unfortunate. I want to get this in first, just to get an error up if someone toggles on dev and get this detection in. I will build on this experience.
We just needsomething now, otherwise it fails silently.
Co-authored-by: ケイラ <mckayla@hey.com>
789c4be
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
EDIT: I made it an
error
instead of a warningTemplates from before
Templates on an old provisioner