- Notifications
You must be signed in to change notification settings - Fork914
fix(agent/agentcontainers): treat customizations as array#18357
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
This PR fixes a mistake from the previous PR#18342. Merged configuration resultsin the customization being an array not an object.This PR also moves `displayApps` from being an array to being an object,like the terraform provider has.
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.
Pull Request Overview
This PR fixes the configuration merging behavior for agent customizations by treating customizations as arrays and updating the DisplayApps type from an array to a map. Key changes include:
- Changing CoderCustomization from a pointer to a slice in the configuration.
- Updating DisplayApps from an array to a map in both the configuration and tests.
- Refactoring related merging logic in the API to accommodate the new configuration structure.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agent/agentcontainers/devcontainercli_test.go | Updated test expectations to reflect changes from a single customization to a slice. |
| agent/agentcontainers/devcontainercli.go | Updated type definitions for CoderCustomization and DisplayApps to match desired schema. |
| agent/agentcontainers/api_test.go | Modified test cases to use a slice for customization and updated DisplayApps assertions. |
| agent/agentcontainers/api.go | Refactored merging logic for DisplayApps using a map and then converting to a slice. |
Comments suppressed due to low confidence (1)
agent/agentcontainers/api.go:1109
- Document the merging behavior here since later customizations override earlier ones when populating the 'displayAppsMap', so future maintainers are aware of this design choice.
for _, customization := range coderCustomization {| displayApps:=make([]codersdk.DisplayApp,0,len(displayAppsMap)) | ||
| forapp,enabled:=rangedisplayAppsMap { | ||
| ifenabled { | ||
| displayApps=append(displayApps,app) |
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.
[nitpick] Consider renaming the 'displayApps' slice (e.g. to 'enabledDisplayApps') to avoid possible confusion with other similarly named variables and improve code clarity.
| displayApps:=make([]codersdk.DisplayApp,0,len(displayAppsMap)) | |
| forapp,enabled:=rangedisplayAppsMap { | |
| ifenabled { | |
| displayApps=append(displayApps,app) | |
| enabledDisplayApps:=make([]codersdk.DisplayApp,0,len(displayAppsMap)) | |
| forapp,enabled:=rangedisplayAppsMap { | |
| ifenabled { | |
| enabledDisplayApps=append(enabledDisplayApps,app) |
Copilot uses AI. Check for mistakes.
agent/agentcontainers/api.go Outdated
| @@ -1099,14 +1099,24 @@ func (api *API) injectSubAgentIntoContainerLocked(ctx context.Context, dc coders | |||
| directory=DevcontainerDefaultContainerWorkspaceFolder | |||
| } | |||
| vardisplayApps []codersdk.DisplayApp | |||
| displayAppsMap:=make(map[codersdk.DisplayApp]bool) | |||
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.
Should we set the defaults here fromterraform-provider-coder?
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.
Good point, I'll add the same defaults here then.
0ef6226 intomainUh oh!
There was an error while loading.Please reload this page.
This PR fixes a mistake from the previous PR
#18342. Merged configuration results in the customization being an array not an object.
This PR also moves
displayAppsfrom being an array to being an object, like the terraform provider has.