10000 chore: refactor dynamic parameters into dedicated package by Emyrk · Pull Request #18420 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

chore: refactor dynamic parameters into dedicated package #18420

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Jun 20, 2025
Merged

chore: refactor dynamic parameters into dedicated package #18420

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
1ca9483
wip
Emyrk Jun 17, 2025
d4dab77
refactor and move dynamic param rendering
Emyrk Jun 17, 2025
c50ee08
fixup FileError test
Emyrk Jun 17, 2025
8152bb7
more wip
Emyrk Jun 17, 2025
247470e
chore: add dynamic parameter unit test
Emyrk Jun 18, 2025
6799896
add another dynamic param
Emyrk Jun 18, 2025
08bffe2
add some comments
Emyrk Jun 18, 2025
f651edc
fmt
Emyrk Jun 18, 2025
67644cc
remove need to constantly send owner id
Emyrk Jun 18, 2025
ea4aa7d
linting
Emyrk Jun 18, 2025
758cd26
refactor loader
Emyrk Jun 18, 2025
e8f1d2d
comments
Emyrk Jun 18, 2025
23840d9
linting
Emyrk Jun 18, 2025
6328899
add idea
Emyrk Jun 18, 2025
cd46813
merge in main
Emyrk Jun 18, 2025
21b24d2
spelling
Emyrk Jun 20, 2025
7a1622d
add comment, remove unused field
Emyrk Jun 20, 2025
db612b3
chore: reprot json error as param diagnostic
Emyrk Jun 20, 2025
000f722
minor fixes
Emyrk Jun 20, 2025
7b80138
only use 1 function, not a 2 step
Emyrk Jun 20, 2025
c1902a7
Revert "only use 1 function, not a 2 step"
Emyrk Jun 20, 2025
d2634e2
only use 1 function, not a 2 step
Emyrk Jun 20, 2025
21ce54b
refactor TemplateVersionParameter to be a converter func
Emyrk Jun 20, 2025
ae82770
Merge branch 'main' into stevenmasley/dynamic_param_pkg
Emyrk Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
linting
  • Loading branch information
@Emyrk
Emyrk committed Jun 18, 2025
commit ea4aa7dc57c409c6966a15cffdd2f8c2813195d5
13 changes: 7 additions & 6 deletions coderd/dynamicparameters/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type Renderer interface {
Close()
}

var ErrorTemplateVersionNotReady = xerrors.New("template version job not finished")
var ErrTemplateVersionNotReady = xerrors.New("template version job not finished")

// Loader is used to load the necessary coder objects for rendering a template
// version's parameters. The output is a Renderer, which is the object that uses
Expand Down Expand Up @@ -91,7 +91,7 @@ func (r *Loader) Load(ctx context.Context, db database.Store) error {
}

if !r.job.CompletedAt.Valid {
return ErrorTemplateVersionNotReady
return ErrTemplateVersionNotReady
}

if r.terraformValues == nil {
Expand Down Expand Up @@ -131,7 +131,9 @@ func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.C
// Renderer caches all the necessary files when rendering a template version's
// parameters. It must be closed after use to release the cached files.
func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*DynamicRenderer, error) {
// If they can read the template version, then they can read the file.
// If they can read the template version, then they can read the file for
// parameter loading purposes.
//nolint:gocritic
fileCtx := dbauthz.AsFileReader(ctx)
templateFS, err := cache.Acquire(fileCtx, r.job.FileID)
if err != nil {
Expand Down Expand Up @@ -174,9 +176,8 @@ type DynamicRenderer struct {
templateFS fs.FS
plan json.RawMessage

ownerErrors map[uuid.UUID]error
currentOwner *previewtypes.WorkspaceOwner
currentOwnerID uuid.UUID
ownerErrors map[uuid.UUID]error
currentOwner *previewtypes.WorkspaceOwner

once sync.Once
close func()
Expand Down
2 changes: 1 addition & 1 deletion coderd/dynamicparameters/static.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,4 @@ func (r *StaticRender) Render(_ context.Context, _ uuid.UUID, values map[string]
}
}

func (r *StaticRender) Close() {}
func (*StaticRender) Close() {}
5 changes: 3 additions & 2 deletions coderd/parameters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ func (api *API) templateVersionDynamicParametersWebsocket(rw http.ResponseWriter
})(rw, r)
}

//nolint:revive // listen is a control flag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a control flag acceptable or desired here if the linter discourages it? Perhaps we should include that justification in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 👍

Copy link
Contributor
@SasSwart SasSwart Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood as much as you've described in the new comment. My question is why this needs to be a "control flag" in this way instead of being extracted from the context inside this endpoint. They're functionally equivalent as far as I can tell. I'm not saying one approach is better.

I'm asking because I'm sure the linting rule was added for a reason and //nolint can be a smell unless it's well justified.

Copy link
Member Author
@Emyrk Emyrk Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I read it, you are right. I could refactor this to remove the control flag now that the dynamicparameters package exists.

I'm going to keep it for now, and I'm going to make an issue to revisit this.
#18478

It is on the parameters board, so I can fix this up in my next maintenance cycle 👍

func (api *API) templateVersionDynamicParameters(listen bool 7B20 , initial codersdk.DynamicParametersRequest) func(rw http.ResponseWriter, r *http.Request) {
return func(rw http.ResponseWriter, r *http.Request) {
ctx := r.Context()
Expand All @@ -78,16 +79,16 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D

err := loader.Load(ctx, api.Database)
if err != nil {

if httpapi.Is404Error(err) {
httpapi.ResourceNotFound(rw)
return
}

if xerrors.Is(err, dynamicparameters.ErrorTemplateVersionNotReady) {
if xerrors.Is(err, dynamicparameters.ErrTemplateVersionNotReady) {
httpapi.Write(ctx, rw, http.StatusTooEarly, codersdk.Response{
Message: "Template version job has not finished",
})
return
}

httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Expand Down
0