Skip to content
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

Cleanup references to os #29

Open
zendern opened this issue Aug 15, 2020 · 6 comments
Open

Cleanup references to os #29

zendern opened this issue Aug 15, 2020 · 6 comments

Comments

@zendern
Copy link
Contributor

zendern commented Aug 15, 2020

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 today we do --os to be able to pass in the operating system you want to target. Realistically the OS is not important but the package manager is more important.

This is much less important now that auto detection is a thing but I would like to possibly do the following things.

  1. deprecate the -os option
  2. add a new -pm --package-manager option that allows for you to pass in yum, dkpg, apt, dnf, etc etc.
  3. update all code paths that refer to os to no longer do that
  • What feature or behavior is this required for?
    Nothing really ... the OS flag has always bothered me a little :)

See these comments here

//Having this be OS is a little weird. It probably should have been just package manager based flag.

cc @bhamail / @DarthHater / @ken-duck

@deadlysyn
Copy link
Contributor

If we don't already have WIP for this, I can give it a shot. I am interested in learning more about Ahab to see if I can plug it into my containerized pipelines.

@zendern
Copy link
Contributor Author

zendern commented Aug 25, 2020

@deadlysyn go for it. Im not aware on anyone working this yet. Please reach out with any questions or open a draft PR if you get that far and we can work through it together. 👍

@deadlysyn
Copy link
Contributor

deadlysyn commented Sep 25, 2020

i've gone through chase/iq.go (mostly) and s/os/package-manager along with related cleanup...wanted to get your thoughts on a couple questions.

context:

the former os or operating value got passed into package methods. that looked like:

// package.go
type IPackage interface {
	ExtractPurlsFromProjectList(string) []string
}

// apt.go, etc...
type Apt struct {
	ProjectList parse.ProjectList
}

func (a Apt) ExtractPurlsFromProjectList(operating string) (purls []string) {
	for _, s := range a.ProjectList.Projects {
		var purl = fmt.Sprintf("pkg:deb/%s/%s@%s", operating, s.Name, s.Version)
		purls = append(purls, purl)
	}
	return
}

passing operating into ExtractPurlsFromProjectList makes sense to satisfy the interface, and looks normal in the apt example (or dnf/yum) but apk.go does not seem to use it at all:

// apk.go
type Apk struct {
	ProjectList parse.ProjectList
}

func (a Apk) ExtractPurlsFromProjectList(operating string) (purls []string) {
	for _, s := range a.ProjectList.Projects {
		var purl = fmt.Sprintf("pkg:alpine/%s@%s", s.Name, s.Version)
		purls = append(purls, purl)
	}
	return
}

is that:

  • intentional, due to difference in required purl format
  • a bug and we should reference os (now packageManager) value in apk.go like the others
  • method parameter might not be needed at all now (redundant when attached to package type?) and the interface + methods should be updated

thoughts? TIA!

@zendern
Copy link
Contributor Author

zendern commented Sep 26, 2020

Re: Os passed in to create the purl path. So its by intention but honestly we effectively have it hardcoded to debian right now. That could possibly be wrong. Since debian purl could in theory be debian or ubuntu based on this documentation.

https://ossindex.sonatype.org/ecosystem/debian

And as you can see alpine and rpm dont have that setup at all.
https://ossindex.sonatype.org/ecosystem/alpine
https://ossindex.sonatype.org/ecosystem/rpm

Probably a question for @ken-duck why that might be different and if we need to accomodate for ubuntu vs debian since we are not today. Or if there is maybe a feature enhancement where we can just not pass it in like we do for alpine/rpm.

cc @deadlysyn

@deadlysyn
Copy link
Contributor

Thanks for the insight @zendern will wait for @ken-duck to provide feedback, but also going to workup an initial PR and link in so there's actual changes/code to review. I'll link that in here, hopefully later today.

Thanks to your pointers, I also found https://github.com/package-url/purl-spec#purl which, if I read correctly, says the OS (or package manager) is the "type" which is required (namespace is optional, but we have stuff like pkg:alpine so I think alpine is "type" from the spec). If that's a correct statement, then we should consistently provide the type field in our output, whether OS or package manager...and from the same doc, using the package manager as the type will be more correct (they give examples of npm, nuget, etc).

Since it's the weekend I'm going to include some of this interpretation in the initial PR, and can fixup as needed after feedback.

@deadlysyn
Copy link
Contributor

deadlysyn commented Sep 27, 2020

So excited to get this submitted, I mistakenly thought Hacktoberfest submissions were already being counted (since they opened up signup page last week). :-D Oh well, at least there are more to do. Let me know if the interface/purl changes are too crazy. TIA for review.

This was referenced Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants