Auto Detect v2

[zendern]

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 #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 #32 thats not going to work. It would also be nice to not have people install which 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

[bhamail]

FWIW, I just came across this little project. Might provide ideas if nothing else: https://github.com/tresf/whut

[deadlysyn]

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.

[deadlysyn]

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!

[zendern]

@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 chocolatey support so honestly i wouldn't worry about it we can cross that bridge if we ever implement #32 .

We would be real close to what whut does if you do add /etc/os-release checking....

This is where Whut does that here https://github.com/tresf/whut/blob/055e9fbb588c5a627a7648870b48c8b5d8c88c76/src/io/tresf/whut/CliParser.java#L74-L88

And then falls back to which and where here. https://github.com/tresf/whut/blob/055e9fbb588c5a627a7648870b48c8b5d8c88c76/src/io/tresf/whut/PkgType.java#L59-L76

[deadlysyn]

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!

[ButterB0wl]

@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 :)

[deadlysyn]

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.

[zendern]

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??

[deadlysyn]

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

[deadlysyn]

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.

[deadlysyn]

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 b) feels like tightly coupling test/code so might be bad

Thoughts?

[deadlysyn]

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.