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

Add ent schemas and project config files #1

Merged
merged 12 commits into from
Apr 23, 2024

Conversation

jhoward-lm
Copy link
Contributor

@jhoward-lm jhoward-lm commented Apr 16, 2024

Initial ent schema definitions.

Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
@jhoward-lm
Copy link
Contributor Author

jhoward-lm commented Apr 22, 2024

@puerco

Added a mixin schema so that all the other schemas get 4 additional fields:

SourceType

Either cdx or spdx to keep track of the original SBOM format

SourceData

Contains the original JSON data of the CDX or SPDX type

CDXExtra

Any CycloneDX fields from the source document that protobom is unable to express, for use in reconstructing the document later

SPDXExtra

Any SPDX fields from the source document that protobom is unable to express, for use in reconstructing the document later

Might be able to be combine CDXExtra and SPDXExtra into just ExtraData or something like that

Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
@puerco
Copy link
Member

puerco commented Apr 22, 2024

Thanks for the PR @jhoward-lm ! Can we scope this PR to just the ent backend? Essentially to just go.mod/dum and everything under backends/ent.

Some of the other parts of the repo it is touching will eventually be handled org-wide so we probably don't need them here. If any of them are important (such as CI :) feel free to open other prs and we can merge them separately.

Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

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

Thanks @jhoward-lm ! Looks good overall just two comments:

  1. I'm not sure about the native format mixins. At the point where we'll interface with the storage backends, none of the data from the native formats will be available to store.

    I feel this needs a little more discussion. As this is likely to change I would rather strip it out for now and merge it after we establish a way to wire it in and what the data could look like.

  2. Second, some minor repo housekeeping comments.

Thanks for working on this!

Comment on lines +5 to +9
// --------------------------------------------------------------
// SPDX-FileCopyrightText: Copyright © 2024 The Protobom Authors
// SPDX-FileType: SOURCE
// SPDX-License-Identifier: Apache-2.0
// --------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

At some point we should start enforcing the boilerplates across the org, probably using the same tools we use in other projects. We usually hold the boilerplates in the hack/boilerplates dir, such as this one: https://github.com/openvex/vexctl/tree/main/hack/boilerplate

Let's not block on this, but if the generator can import these lines from the boilerplates dir it would be great as we would only need to maintain one file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other question about the file name and license text content of the file header. I had trouble getting an accurate file name for the individual files being generated, but what about the license text?

Isn't hack/ typically used for things like dev scripts? This template is specifically scoped for and tied to ent code generation, and referenced as a relative path in entc.go. I guess if we feel confident this backends/ent/template path will never change, maybe entc.go could be changed to something like

// from
gen.MustParse(gen.NewTemplate("header").ParseFiles("template/header.tmpl"))

// to
gen.MustParse(gen.NewTemplate("header").ParseFiles("../../hack/boilerplate/header.tmpl"))

My thought was having it as a relative subdirectory seemed cleaner to me, and aligns with the ent codebase structure

Copy link
Member

Choose a reason for hiding this comment

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

We can wire this in later, it's just that in the future having a single boilerplate template if possible will be better.

Isn't hack/ typically used for things like dev scripts?

We are carrying a lot of automation from the kubernetes world that relies on the ./hack dir, some of that includes the boilerplate checks. We could have it elsewhere but if we can just reuse some of that why not :)

Copy link
Member

Choose a reason for hiding this comment

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

I am in general not very fond of checking in a lot of IDE config but I don't mind if you'd like to have it. But if we want to keep it it should go at the root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, I had the file name and license text in the file headers, which the snippet was auto populating, based on the guidance from here stating file name is required and here outlining how to add the Apache-2.0 license to source files (example). Should we consider adding them back in?

To me, the snippet file is there not so much as IDE config as it is for developer convenience, the goal being to reduce copy-paste errors and human error in general. Before removing the file name generation, if using VSCode (which I believe most devs are these days), you could just start typing "spdx", "license", or "header" at the top of the file to auto-populate with the current year and name of the file. If it stays, it needs to remain in .vscode/ according to their docs.

That being said, just to explain my thought process and not to advocate for anything, I typically include VSCode configs (.vscode/) and .editorconfig in my projects as a means to level-set and automate the consistency of code quality from contributors. It helps with code reviews when there is a level of confidence that the code is automatically formatted during development so the focus can be on the real content of the changes. I can also see the counter-argument of being too prescriptive.

Copy link
Member

Choose a reason for hiding this comment

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

We chatted and we're dropping the filename from the header (the requirement in the link above is for SBOMs). Since we'll have fixed boilerplates we don't need the editor automation .

Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
Signed-off-by: Jonathan Howard <jonathan.w.howard@lmco.com>
@jhoward-lm jhoward-lm mentioned this pull request Apr 23, 2024
Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

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

This is awesome @jhoward-lm , thanks for your hard work on this!
/lgtm

@puerco puerco merged commit b6eafea into protobom:main Apr 23, 2024
@jhoward-lm jhoward-lm deleted the ent-schemas branch April 23, 2024 23:43
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.

2 participants