- Notifications
You must be signed in to change notification settings - Fork927
fix(agent): fix script filtering for devcontainers#18635
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
fix(agent): fix script filtering for devcontainers#18635
Conversation
| slog.F("config_path_param", configPath), | ||
| ) | ||
| } | ||
| // Send logs via agent logging facilities. | ||
| logSourceID := api.devcontainerLogSourceIDs[dc.WorkspaceFolder] |
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.
Review: We were accessing this outside the mutex so I moved it just in case due to change in how weInit() the API which allows part of the API to be used beforeInit() is called with input that changes this map.
This was broken in#18630.
8402237 to18bfab2Compare| agentcontainers.WithSubAgentClient(agentcontainers.NewSubAgentClientFromAPI(a.logger, aAPI)), | ||
| ) | ||
| _, devcontainerScripts := agentcontainers.ExtractDevcontainerScripts(manifest.Devcontainers, manifest.Scripts) |
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.
Is there any reason to call this function twice?
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.
Mainly I wanted to reduce variable scope to avoid similar issues as introduced in the previous PR. And make it clear that scripts from earlier shouldn't be used here. It's a lightweight call but I can revert it if you prefer.
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 have no issue with the duplication just wanted to ensure I knew the reason why, all good!
8ee2668 intomainUh oh!
There was an error while loading.Please reload this page.
This was broken in#18630.