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

Move output package from /internal to /pkg #199

Closed
dburriss opened this issue Feb 8, 2023 · 3 comments
Closed

Move output package from /internal to /pkg #199

dburriss opened this issue Feb 8, 2023 · 3 comments

Comments

@dburriss
Copy link

dburriss commented Feb 8, 2023

Please let me know if I am missing something here. I am very new to Go lang.

I suggest changing the output package to a public package (move to /pkg folder).

The main reason for this change suggestion is that the public func DoScan(actions ScannerActions, r *output.Reporter) (models.VulnerabilityResults, error) function in the osvscanner package exposes an internal type in its public API. Namely, output.Reporter from the internal output package.

Another reason for this change is that the ouput package relies on public packages ie. the models package.

flowchart LR

    subgraph pkg
    config
    osvscanner-->models
    osv-->models
    osv-->lockfile
    end

    subgraph internal
    sbom
    semantic
    output
    end

    osvscanner-->output
    output-->models
    output-->osv
    config-->output
    semantic-->lockfile
    osvscanner-->sbom
Loading

Making the output package part of public packages by moving it to /pkg makes the public API usable by making it depend on only public types.

flowchart LR

    subgraph internal
    sbom
    semantic
    end

    subgraph pkg
    config
    osvscanner-->models
    osv-->models
    osv-->lockfile
    osvscanner-->output
    output
    output-->models
    output-->osv
    end


    osvscanner-->sbom
    config-->output
    semantic-->lockfile
    
Loading

Due to the reliance of semantic.Parse and semantic.MustParse on filelock.Ecosystem type, there still will exist a 2-way dependency between pkg and internal packages. This could be solved by moving the 2 above mentioned functions into the lockfile package. A possible destination is the parse.go file. Then parse.go could instead import the semantic package, with a few changes (making the semver parsing public internal).

flowchart TB

    subgraph pkg
    config
    osvscanner-->models
    osv-->models
    osv-->lockfile
    osvscanner-->output
    output
    output-->models
    output-->osv
    end

    subgraph internal
    sbom
    semantic
    end

    config-->output
    lockfile-->semantic
    osvscanner-->sbom
Loading

I will take a look at what this would look like in a Draft PR if anyone is interested in the changes? I made the 1st change already because I needed it but happy to PR back if something thinks this reasoning makes sense.

@dburriss
Copy link
Author

dburriss commented Feb 8, 2023

Looking into it semantic.Parse and semantic.MustParse are only used in tests.

@another-rex
Copy link
Collaborator

Thanks for pointing this out! I believe just moving reporter out of internal, into it's own package, rather than the entire output package would be preferred.

@another-rex
Copy link
Collaborator

Resolved by: #341

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