fix(agent/agentcontainers): treat customizations as array #18357
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 `displayApps` from being an array to being an object, like the terraform provider has.
There was a problem hiding this comment.
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 {
Comment on lines
+1116
to
+1119
| displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) | ||
| for app, enabled := range displayAppsMap { | ||
| if enabled { | ||
| displayApps = append(displayApps, app) |
There was a problem hiding this comment.
[nitpick] Consider renaming the 'displayApps' slice (e.g. to 'enabledDisplayApps') to avoid possible confusion with other similarly named variables and improve code clarity.
Suggested change
| displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) | |
| for app, enabled := range displayAppsMap { | |
| if enabled { | |
| displayApps = append(displayApps, app) | |
| enabledDisplayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) | |
| for app, enabled := range displayAppsMap { | |
| if enabled { | |
| enabledDisplayApps = append(enabledDisplayApps, app) |
Copilot uses AI. Check for mistakes.
mafredri
approved these changes
Jun 13, 2025
agent/agentcontainers/api.go
Outdated
| @@ -1099,14 +1099,24 @@ func (api *API) injectSubAgentIntoContainerLocked(ctx context.Context, dc coders | |||
| directory = DevcontainerDefaultContainerWorkspaceFolder | |||
| } | |||
|
|
|||
| var displayApps []codersdk.DisplayApp | |||
| displayAppsMap := make(map[codersdk.DisplayApp]bool) | |||
There was a problem hiding this comment.
Should we set the defaults here from terraform-provider-coder?
There was a problem hiding this comment.
Good point, I'll add the same defaults here then.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.