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

Merged
merged 1 commit into from
Nov 21, 2024
Merged

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 command in Zarf

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% <ø> (ø)

🚨 Try these New Features:

@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
@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 canceled.

Name Link
🔨 Latest commit 00c5d98
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/673f364bafec7b0008c7bf04

@AustinAbro321
Copy link
Contributor Author

Removing the needs design flag, as I simplified this PR to only change where the schema is generated, rather than generating a schema for the apiVersion

@phillebaba phillebaba force-pushed the separate-schema-generation branch from 8c4bdb1 to 08fc321 Compare November 21, 2024 10:32
@AustinAbro321 AustinAbro321 force-pushed the separate-schema-generation branch from 08fc321 to 00c5d98 Compare November 21, 2024 13:31
@AustinAbro321 AustinAbro321 added this pull request to the merge queue Nov 21, 2024
Merged via the queue into main with commit 2e66463 Nov 21, 2024
38 checks passed
@AustinAbro321 AustinAbro321 deleted the separate-schema-generation branch November 21, 2024 14:20
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