-
Notifications
You must be signed in to change notification settings - Fork 18
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto Detect v2 #35
Comments
FWIW, I just came across this little project. Might provide ideas if nothing else: https://github.com/tresf/whut |
I believe which (or command -v) should be a shell built-in...so not require installing things for POSIX compatible shells -- though I guess that still wouldn't work for Windows (or are we talking WSL?). $ which which
which: shell built-in command
$ which command
command: shell built-in command I could switch this to do the /etc/os-release check vs which/command -v but need to figure out how that works on Windows. |
If no one has WIP, I can help get this started. Thinking something along the lines of @ButterB0wl mentioned in #34 although https://github.com/tresf/whut looks cool too! |
@deadlysyn yah i think Windows wise it'll work on WSL since that is just ubunutu :) but something we will have to figure out if we ever do We would be real close to what This is where Whut does that here And then falls back to |
I want to wait for #29 to be merged before committing any of this since there's some overlap that will cause conflicts. Just to be sure I understand the desired implementation... The comments in #34 suggest simply running through the package commands and using the first that succeeds might work. That eliminates which and seems simple in a good way. The stack exchange link in #34 as well as the whut reference parse /etc/*-release and use that map to determine which commands to run. That feels fancy, but also more complex...though perhaps better performing. Do you prefer a simple "package command switch" or "os-release parsing" approach? Thanks! |
@deadlysyn Yeah that's a pretty good assessment of the two approaches I would try the try/except each package manager listing against the container first and see how that goes. I don't think it would be slow since the wrong command would immediately error out Additionally I foresee issues around edge cases of parsing those directories as well as getting to them in the first place would require running multiple commands in sequence against the container after it's been instantiated I think it's a relatively simple problem so lets go with the simple approach :) |
Here's one version of os-release parsing with no external dependencies (not even a shell, like whut had): https://github.com/deadlysyn/go-os-parse I'm sure it can be simplified, there's some paranoia included. Does that look useful or crazy? I'll workup a version of the command approach. |
Nice !!! That looks awesome to me!! Only comment I have is should probably have some tests around it. Maybe integration test would be cool. Spin up a docker image and run it for each os?? |
I cribbed the docker examples in this repo to get tests working for go-os-parse. I have the detector code integrated in my branch, and the docker-based tests pass which makes me fairly confident it's working as expected (particularly since the API didn't change just the implementation). However, I'm loathe to change code + tests at the same time...but have hit a snag. The detector_test.go stuff is a bit opaque to me so I might be reading it wrong. It's failing on all but the expected error case. However, I don't think it can pass with the new implementation since it doesn't mock os-release (that's effectively what the docker tests do). Unless I misunderstand, I think this test will need refactored, since it runs locally and so always just uses /etc/os-release from the developer system. Wanted to see if you have ideas on how that could be refactored to add value, or if it is covered by docker_test.go? TIA |
OK I think I finally get the exit 0/1 bits as mocking which's behavior... Some times it's harder for me to wrap my head around tests than the real code. Learning a lot here! Still appreciate input on appropriate level of test coverage. I almost feel like the docker tests are the best way to wrap the new os-release parsing, but a unit test that somehow mocks that w/o a docker dependency might be cool. |
idea: DetectPackageManager adds a variadic argument. If not passed, it defaults to parsing os-release as usual. If passed, it's used as the distro ID to ensure the matching works as expected. Then the test can just inject whatever OS it wants to mimic. a) seems like some "dependency injection" magic so potentially good Thoughts? |
Don't mind to rework as needed, figured code would be easier to review than words... Submitted the variadic paramater approach. I'm still not sure if that kind of hook to make testing easier os OK or an example of tight coupling. |
Thanks for creating an issue! Please fill out this form so we can be
sure to have all the information we need, and to minimize back and forth.
What are you trying to do?
So we added auto detect of your os/package manager in Auto detect lets try again #24 but it currently has a dependency on
which
being installed. This is fine for now but with plans to maybe support windows images in Add windows docker support when using Chocolatey #32 thats not going to work. It would also be nice to not have people installwhich
in there image if they dont really need to.What feature or behavior is this required for?
Less dependencies in the auto detect process.
How could we solve this issue? (Not knowing is okay!)
Maybe regex?? Maybe it looks something more like this https://unix.stackexchange.com/a/46086
cc @bhamail / @DarthHater / @ken-duck
The text was updated successfully, but these errors were encountered: