Issue 35

[deadlysyn]

Update package manager detection to use /etc/os-release.

This pull request makes the following changes:

• Parse /etc/os-release to determine appropriate package manager
• Remove dependency on which (or any external shell/command) for detection
• Update unit test to mock os-release content vs which return code

API is unchanged so behavior should be the same. Docker functional tests still pass as expected.

NOTE: Since #29 has not been merged and there is overlap, I've pulled those changes into this branch to try and avoid merge conflict fun. detector.go and detector_test.go are all that contain new changes specific to this issue.

It relates to the following issue #s:

• Fixes #35

cc @bhamail / @DarthHater / @zendern

[bhamail]

Looks like some of the prior PR's I merged caused some conflicts. I will try to resolve the conflicts. Apologies in advance if I make a bigger mess.

[deadlysyn]

@bhamail no worries, you are not alone in merge conflict purgatory :-D i could also help sort it out if you want -- let me know how it goes. i might be able to jump through the hoops faster in this specific case if it relates to change i made recently. should i just pull in master on my side and sort it out?

aside from that, this has turned into a bigger issue than i first imagined...detecting os based on /etc/os-release was easy enough, but the inconsistent output format of dnf and yum (newer versions some times seem to split package names/versions across multiple lines in rare cases, which breaks parsing...we could piece those lines together, but it feels brittle and makes me think GIGO so we should possibly change how we inject the package data).

because that is a giant can of worms i am tempted to just help you get the os-release parsing/detection conflicts sorted, back out the test changes, and then maybe we do another PR that is about adding back the extended tests (which uncovered the package version parsing issue that's been lurking all along) + work through an appropriate fix. there's different ways we can do it, and while rpm queryformat feels technically right changes usage so probably needs larger discussion/review.

[bhamail]

:) Well, I don't pretend to know why the PR I created against your branch doesn't even show changes to the files listed as conflicting: detector.go and detectory_test.go, but here is it in case it is of any help: https://github.com/deadlysyn/ahab/pull/1

I'm guessing ignoring that PR and fixing it directly is no worse. ;)

[deadlysyn]

Took care of the merge conflicts, and added what I think is the minimal workaround for the dnf/yum parsing issue. It's not ideal, because it just skips the bad lines (that package won't be scanned), but the current behavior is a crash from indexing out of bounds. I see the last commit as a bandaid to prevent the crashes described above, but we should create a new PR to discuss and agree on the real solution.

[zendern]

and added what I think is the minimal workaround for the dnf/yum parsing issue

So I agree with all that you've mentioned around the issue. If you wouldn't mind opening an issue to deal with the actual issue. I think the workaround is good enough for now but moving over to rpm sounds reasonable and really maybe we just make that a 1.0 level change.

are there time limits to worry about in the current CI setup? even w/o a rate limit, more tests obviously add tiime. aside from causing longer builds, not sure if there are hard limits we need to keep in mind. it's all running with t.Parallel() but also not sure how parallelism is limited in pipelines. 🤔

Ummmm..... not sure how that would be. We do the same make test locally where you were getting rate limited and CI. You have to explicitly pass in the username and password. So I'm not sure why in this PR the tests did in fact pass.

do the CI tests currently run in such a way that they use an account/token for OSSI access? even if this odd error is chased down, i think the additional tests will still require token access to ever succeed. i was thinking we could refactor the dockerfiles to use env vars to feed that to ahab, but that would mean the CI system needs configured house secrets and devs need .envrc or some way to consistently set the same. is this worthwhile, or should i just pair back the tests?

Yah as I alluded to above I dont believe we have anything like that setup from what I know. Maybe @bhamail or @DarthHater can go poking and looking at the circleCI config magic for us?? But that could be a good addition if we dont have anything to get around this.

[bhamail]

do the CI tests currently run in such a way that they use an account/token for OSSI access? even if this odd error is chased down, i think the additional tests will still require token access to ever succeed. i was thinking we could refactor the dockerfiles to use env vars to feed that to ahab, but that would mean the CI system needs configured house secrets and devs need .envrc or some way to consistently set the same. is this worthwhile, or should i just pair back the tests?

CircleCI has a nice mechanism for passing secrets via environment variables. We use it often.

Related to Issue #27: Also, if we decide to, we could leverage Viper to read the ossi credentials (e.g. by default, the same file used by nancy (see nancy link below). Can override in tests to some other file, or env var, etc). We already use Cobra commands, and Viper integrates nicely with Cobra. Here's some of what nancy does to read credentials via Viper. Viper supports the following precedence for reading values:

1. explicit call to Set
2. flag
3. env
4. config
5. key/value store
6. default

[bhamail]

I'll see if I can make some progress with adding Viper to ahab

[bhamail]

I merged the Viper/config stuff into master.