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 a pre-commit for validating zarf schema #379

Merged
merged 5 commits into from
Mar 14, 2022
Merged

Conversation

YrrepNoj
Copy link
Contributor

@YrrepNoj YrrepNoj commented Mar 10, 2022

Description

Add a pre-commit hook that will validate that the ZarfInitConfig ./zarf.yaml confirms to the schema defined at ./zarf.schema.json. This will help prevent issues where developers update the init config without updating the schema.

Related Issue(s)

Fixes #216

@YrrepNoj YrrepNoj self-assigned this Mar 10, 2022
Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

Lgtm, just tested and it rejected as expected.

@YrrepNoj
Copy link
Contributor Author

This is the result of the pre-commit hook if you change the zarf.yaml without changing the schema definition.

Screen Shot 2022-03-10 at 4 06 17 PM

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

We will need to look into this some more. This only checks the zarf.yaml for init packages and isn't fully verifying that the generated schema actually is correct. Would it be simpler to just run zarf tools config-schema > zarf.schema.json in a pre-commit step?

@RothAndrew
Copy link
Contributor

We will need to look into this some more. This only checks the zarf.yaml for init packages and isn't fully verifying that the generated schema actually is correct. Would it be simpler to just run zarf tools config-schema > zarf.schema.json in a pre-commit step?

It's not quite this simple, but yes, it should be along these lines. pre-commit is already smart enough to know that a changed file means a failed hook.

@RothAndrew
Copy link
Contributor

I would like to merge this PR, as what it is doing is still valuable, just without closing #216.

@RothAndrew
Copy link
Contributor

Though, it would be better to have the hook check all zarf.yaml files that get changed in the repo, rather than just the root one. I haven't looked closely at the hook's capabilities, but if it does what I think it should we can change the work here to run the hook an all files named zarf.yaml

@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Mar 10, 2022

This hook will run against any file named zarf.yaml that has been staged for commit

Here it's running against one of the example packages zarf.yaml
Screen Shot 2022-03-10 at 4 55 36 PM
.

@RothAndrew
Copy link
Contributor

Cool, then yeah, I'd like to merge this, just without closing #216

@YrrepNoj
Copy link
Contributor Author

Would it be simpler to just run zarf tools config-schema > zarf.schema.json in a pre-commit step?

If we're going to do something like that I would rather do it in an automated pipeline instead of a pre-commit hook.

  1. I don't want the pre-commit to be dependent on having the CLI built and building it in the hook would be too slow which seems like a lot of friction.
  2. We could probably do it without the zarf CLI but that would require dependencies to be manually installed which is also too much friction IMO.

@jeff-mccoy
Copy link
Contributor

It doesn't need to "be built", it could be go run cli/main.go tools. But 🤷

@RothAndrew
Copy link
Contributor

The pre-commit hooks don't have to be run locally, though once we turn on verification of them in the pipeline it will be a lot easier to run them locally then get the feedback loop on what still needs to be fixed from the pipeline.

My vision for the pipeline is that it will run pre-commit run -a which will fail if any of the hooks fail, which would easily and automatically get us linting, checking for secrets, code style (after we add a styler similar to how Prettier or Black work), and other great automated checks.

@RothAndrew
Copy link
Contributor

Doing so will require certain CLI tools to be installed on the developer's machine, such as golangci-lint.

jeff-mccoy
jeff-mccoy previously approved these changes Mar 11, 2022
@YrrepNoj YrrepNoj force-pushed the 216-schema-precommit branch from ab81956 to a7c18ef Compare March 11, 2022 17:51
@YrrepNoj
Copy link
Contributor Author

Screen Shot 2022-03-11 at 12 46 05 PM

pre-commit hook now verifies the zarf-schema is up-to-date if the cli/types/types.go file is ever updated. It also still verifies any modified zarf.yaml conforms to the schema.

@YrrepNoj YrrepNoj merged commit 29f2057 into master Mar 14, 2022
@YrrepNoj YrrepNoj deleted the 216-schema-precommit branch March 14, 2022 14:46
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
* add pre-commit for validating zarf schema
* pre-commit hook to make sure zarf schema is current
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.

Pre-commit hook for ensuring the Zarf schema is up to date
3 participants