feat(docker-win): add main for windows #271
Conversation
rajrohanyadav
commented
Feb 4, 2025
rajrohanyadav
commented
- This modifies the module to enable adding windows functionality.
- Certain decisions made to minimise duplication.
- Cgoups will only be supported for linux. Windows and darwin will use dockerAPI.
src/config/config.go
Outdated
| @@ -6,7 +6,7 @@ type ArgumentList struct { | |||
| args.DefaultArgumentList | |||
| HostRoot string `default:"" help:"If the integration is running from a container, the mounted folder pointing to the host root folder"` | |||
| Fargate bool `default:"false" help:"Enables fetching metrics from ECS Fargate. If enabled no metrics are collected from cgroups. Defaults to false"` | |||
| UseDockerAPI bool `default:"false" help:"Enables fetching metrics from Docker API. If enabled no metrics are collected from cgroups. This option is ignored if cgroupsV1 are detected"` | |||
| UseDockerAPI bool `default:"false" help:"Enables fetching metrics from Docker API. If enabled no metrics are collected from cgroups. This option is ignored if cgroupsV1 are detected or running on any OS other than linux"` | |||
There was a problem hiding this comment.
When cgroupsV1 is detected we set it to false and when running on any OS other than linux we set it to true. Is that right? I wonder if we could be clearer somehow 🤔
There was a problem hiding this comment.
Attempted to improve it in 19d53c8
I believe we could default it to true for next releases, but not sure if that will be a breaking change 💭
Let's keep it this way for this release just to de-couple things, and I'll try to see if it can be defaulted to true for all.
src/util/util_all.go
Outdated
| func UpdateDockerAPIArg(dockerAPIArg bool) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Maybe we could improve the function name, the Update part suggest some modification and it is merely returning a value (even if it is used to modify the args right now)
There was a problem hiding this comment.
Improved the function name in 023d6cc
Hmm, I didn't want someone to make a mistake of reading args somewhere in the program and then finding that it's value is different from the main caller. So, I updated the arg globally to avoid such mistakes. WDYT 🤔
src/docker.go
Outdated
| // we always use the docker API for OSes other than Linux | ||
| args.UseDockerAPI = util.UpdateDockerAPIArg(args.UseDockerAPI) |
There was a problem hiding this comment.
We could also avoid modifying the args if possible. Maybe something like:
useDockerAPI := util.shouldUseDockerAPI(args.UseDockerAPI)We could also move the comment above to the OS specific implementation of the function.
There was a problem hiding this comment.
Added a comment in 023d6cc
For args, the same reason as I added above. I could pass across this var, but just think someone (most probably me 3 months later 😄 ) might make a mistake with different global and local values.
Yes, more PRs will be there. Just wanted to help the reviewers with smaller PRs :) |
