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

chore: separate schema generation #2886

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

AustinAbro321
Copy link
Contributor

@AustinAbro321 AustinAbro321 commented Aug 14, 2024

Description

This moves schema generation to it's own folder rather than being a part of the Zarf binary. This moves the schema's to their own folder to help keep the repo clean as the number of schemas will grow as we introduce schema versions. However this does not delete the zarf.schema.json at the root of the repo as we've pointed users to that file in the docs to get schema validation in VS code.

This doesn't remove the invopop/jsonschema from the Zarf CLI, however there is only one place left it is used. Likely this can be removed in a future PR and replaced with runtime validation.

func (ZarfComponentImport) JSONSchemaExtend(schema *jsonschema.Schema) {

Relates to #2788

Checklist before merging

@AustinAbro321 AustinAbro321 changed the title Separate schema generation chore: separate schema generation Aug 14, 2024
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/cmd/internal.go 0.00% <ø> (ø)

Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
}

var genTypesSchemaCmd = &cobra.Command{
Use: "gen-types-schema",
Copy link
Contributor Author

@AustinAbro321 AustinAbro321 Aug 14, 2024

Choose a reason for hiding this comment

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

I didn't build a replacement for this as I don't think it's needed. I'm not sure what the original use case was for adding this, though I don't think it makes sense to have an internal command generating a schema for types users shouldn't be creating.

@AustinAbro321 AustinAbro321 marked this pull request as ready for review August 14, 2024 19:14
@AustinAbro321 AustinAbro321 requested review from a team as code owners August 14, 2024 19:14
@phillebaba
Copy link
Member

Good jobs getting this done. A couple of comments.

Is the docs changes related to these changes or are we fixing this because we found out it was wrong during this work?

We wont remove any dependency in the Zarf CLI in this change. Just making sure because the description makes it seem like we are reducing the amount of dependencies.

The last thing is that I think this code should go in a tools directory located in the root of the project. It would be good if the shell script that goes along with it would also be located with the Go code instead of in the hack directory. Here is an example of how other projects solves this.
https://github.com/openshift/origin/tree/master/tools

I would like @schristoff input on this type of project structure.

@AustinAbro321
Copy link
Contributor Author

Is the docs changes related to these changes or are we fixing this because we found out it was wrong during this work?

It's related to this work. Essentially we're copying the alpha schema to a different folder. I'm not deleting the original zarf.schema.json since many users rely on it. However once we're in v1 and users no longer need that original zarf.schema.json I'm thinking we can move all the schemas to the tools folder

We wont remove any dependency in the Zarf CLI in this change. Just making sure because the description makes it seem like we are reducing the amount of dependencies.

Yeah my description was confusing, fixed it.

The last thing is that I think this code should go in a tools directory located in the root of the project. It would be good if the shell script that goes along with it would also be located with the Go code instead of in the hack directory. Here is an example of how other projects solves this.

I would agree

@AustinAbro321 AustinAbro321 marked this pull request as draft August 15, 2024 13:13
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
@AustinAbro321 AustinAbro321 marked this pull request as ready for review August 15, 2024 16:44
Copy link

netlify bot commented Sep 3, 2024

Deploy Preview for zarf-docs ready!

Name Link
🔨 Latest commit fd0ed8c
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/66d722b44fe90500075d6c1d
😎 Deploy Preview https://deploy-preview-2886--zarf-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants