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

feat: create osvschema go bindings package #292

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Oct 9, 2024

This migrates some of the "schema" based types that currently live in osv-scanner over to here since they're related to the underlying OSV schema rather than the scanner specifically.

Notably to help avoid dependency cycles we now have a dedicated constants package whose sole responsibility is to house types and constants for representing enums defined in the schema with no logic whatsoever; this will ensure that this package is always a leaf in the dependency tree, preventing circular dependencies in libraries (which notably we have now in the current osv-scanner/models package).

This also introduces an ecosystem package which houses our concept of an parsed ecosystem as implicitly defined in the spec - that is, a struct made up of an ecosystem name and an optional suffix. Note the underlying Ecosystem type actually lives in constants which might seem weird at first but 1. prevents cycles as mentioned above, and 2. avoids the weird ecosystem.Ecosystem situation.

Currently this is being introduced as a port from osv-scanner - the fact that there are missing constants will be addressed in a follow-up pull request, along with other changes such as updating the validation/schema.json and (hopefully) creating some automation to help keep everything in sync.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 9, 2024

@another-rex I think this is actually pretty much good to go as a package, but would appreciate you doing a check as I forgot the top level directory/package structure we actually discussed, and also not 100% confident I've setup the package correctly from a Go POV 😅

@andrewpollock
Copy link
Collaborator

Something to consider is the developer experience around onboarding a new ecosystem, or how we'll ensure consistency and accuracy in All The Places...

Right now we're looking at:

I think if that blows out to include

  • packages/go/constants/constants.go

we're also going to need to look at what scaffolding to put in place to ensure we maintain consistency.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 11, 2024

Yup that's exactly what I'm going to be working on next 😄

packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
@another-rex
Copy link
Collaborator

@another-rex I think this is actually pretty much good to go as a package, but would appreciate you doing a check as I forgot the top level directory/package structure we actually discussed, and also not 100% confident I've setup the package correctly from a Go POV 😅

You might need to add another subfolder called osv-schema under go, and name the module github.com/ossf/osv-schema/packages/go/osv-schema, I'm not sure on that either though.

@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 20, 2024

name the module github.com/ossf/osv-schema/packages/go/osv-schema

I'm hoping not because that feels like an annoying amount of nesting, but also the linter is using github.com/ossf/osv-schema/linter, so at least hopefully we might be able to drop the ...

na you must be right, cause otherwise how else could Go actually find the files? (the linter doesn't actually have a public interface, so it doesn't strictly need to follow the convention which is why it's fine)

@G-Rath G-Rath mentioned this pull request Oct 21, 2024
packages/go/constants/constants.go Outdated Show resolved Hide resolved
packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
packages/go/constants/constants.go Outdated Show resolved Hide resolved
oliverchang pushed a commit that referenced this pull request Oct 21, 2024
This setups up a workflow for running the `osv-linter` tests for pull
requests - it looks like there are also tests for the Python-based
tools, which I'll look into as a follow up.

This'll make it easier to later add CI for #292

Signed-off-by: Gareth Jones <jones258@gmail.com>
@G-Rath G-Rath force-pushed the add-packages branch 2 times, most recently from 5fe0482 to 30bef0a Compare October 21, 2024 02:18
@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 21, 2024

I've opened #303 adding a script that handles automatically updating the constants

packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
packages/go/ecosystem/ecosystem.go Outdated Show resolved Hide resolved
//
// For example, "Debian:7" would be parsed into Parsed{Ecosystem: constants.EcosystemDebian, Suffix: "7"}
//
// [spec]: https://ossf.github.io/osv-schema/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be more specifically pointing at https://ossf.github.io/osv-schema/#affectedpackage-field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, but it didn't feel right as this isn't about the affected field itself, it's about the "ecosystem" type/value which happens to be used by the affected field - I really think ultimately the ecosystems table should have its own dedicated header as it's a very important part of the doc yet you currently can't directly link to it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see issue making the ecosystems table to have its own link. @oliverchang what do you think?

Copy link
Collaborator

@another-rex another-rex Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I also got a few requests from others to make it easier to link to the ecosystems table, let's go ahead and add an anchor to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a go at a PR for that then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@another-rex
Copy link
Collaborator

I talked with @oliverchang offline and I think we should remove Parsed for now from the osv-schema repository so that this repository only contains types, constants, and structs with 0 code.

(and probably move Parsed into osv-scanner instead, but that can be a later decision).

another-rex pushed a commit that referenced this pull request Oct 28, 2024
This introduces new dedicated types for the name of an ecosystem (which
is an enum) and an ecosystem suffix (which is a string starting with
`:`), along with renaming the existing `ecosystem` type to
`ecosystemWithSuffix` (+ adding it).

While these types are not actually used in the schema due to JSONSchema
currently does not have a way of specifying that a string is made up of
two types, having them defined enables a lot of automation as the
current `pattern` based type cannot be used to get a finite list of all
possible ecosystems defined by the spec.

As an example, here's a [TypeScript
playground](https://www.typescriptlang.org/play/?#code/C4TwDgpgBAogxgewM4icCBbKBeKByOAJwEN0kA6ASwTygB98BxG+-AWWIDcIA7PAWABQQ0JFiIUaTAHVKwABYAFQhABmlAB45xyVOiwMABgBIA3vF1SMAXwBcZtIUo8A5tYfAnr64YDcQoQATCDgAG2IVKFUAVx44YGoeKJ4ACg1bHUl9WQVlNU0ASgzOBEpA-2FBVVSCEjIqBFsARgAmPAKhapTa0ggKanbOmqJe-sbByud0QlViOGhFCKQIQIsszChTISgoEMt9DLW9TAqdpGjVdXSoR2cXCusgA)
showcasing how you could use the types that would be automatically
generated from this new schema structure using
[`json-schema-to-typescript`](https://www.npmjs.com/package/json-schema-to-typescript)
(with a manually defined `EcosystemSuffix` type, though we can later
introduce that for TypeScript specifically by adding a `tsType`
property).

Notably, landing this will unblock me adding automation to ensure that
all lists of the ecosystem name are in sync (notably, the table in the
docs, the enum + pattern here, and soon the constants being added in
#292)

Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
… from a string in JSON

Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
Signed-off-by: Gareth Jones <jones258@gmail.com>
@G-Rath
Copy link
Contributor Author

G-Rath commented Oct 29, 2024

Ok so have removed the ecosystem package, and for now osvschema has just constants - I'll do another pull request exploring add structs, since they can be more complex and the ones we've got in osv-scanner do have some logic for marshalling JSON and YAML

@G-Rath G-Rath changed the title feat: create new Go packages feat: create osvschema go bindings package Oct 29, 2024
@oliverchang oliverchang merged commit ad55659 into ossf:main Oct 29, 2024
4 checks passed
@G-Rath G-Rath deleted the add-packages branch October 29, 2024 22:24
oliverchang pushed a commit that referenced this pull request Nov 5, 2024
This introduces a new script to make it easier to ensure all "lists of
ecosystems" within this codebase remain up to date, including:
  - the table in `docs/schema.md` 
  - the Go constants being introduced in #292
- the JSON schema in `validation/schema.json` (both the pattern and the
enum being introduced in #296)

To make it a bit easier, I've introduced a top-level `ecosystems.json`
which is a map of defined ecosystems and a markdown description, sorted
alphabetically (which the script also ensures) - I felt this was easier
than trying to extract the list from markdown or another source, though
it does mean double quotes need to be manually escaped.

I went with JSON as it can be read without requiring an external
dependency, though if we use Python 3.11 we could switch to `toml`
instead as that ships with `tomllib`

Example of the workflow output:


![image](https://github.com/user-attachments/assets/aaff0cd4-6387-497f-9869-62ac1b839e58)


![image](https://github.com/user-attachments/assets/057fb2e1-e2ca-4f9b-a704-c116ed69a69f)

---------

Signed-off-by: Gareth Jones <jones258@gmail.com>
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

Successfully merging this pull request may close these issues.

5 participants