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 Schema validator #176

Merged
merged 10 commits into from
Nov 25, 2024
Merged

Add Schema validator #176

merged 10 commits into from
Nov 25, 2024

Conversation

oleander
Copy link
Contributor

This PR introduces JSON schema validator for dip.yml config files to help users catch configuration errors early and ensure their configs follow the expected format.

Key Changes

  • Added JSON schema
  • Introduced new dip validate CLI command
  • Added json-schema gem dependency
  • Integrated validation into the config loading process
  • $schema: https://raw.githubusercontent.com/bibendi/dip/refs/heads/master/schema.json allows your editor to lint the dip.yml on-the-fly, if configured for it.

Features

  1. New dip validate Command

    • Users can validate their dip.yml configuration by running dip validate
    • Provides detailed error messages when validation fails
  2. Schema Support in dip.yml

    • Users can specify schema in their config using $schema property (the URL does not exist until the PR gets merged):
    $schema: https://raw.githubusercontent.com/bibendi/dip/refs/heads/master/schema.json
  3. Automatic Validation

    • Configuration is automatically validated when loaded
    • Helps catch issues early in the development process

Testing

  • Added comprehensive test coverage for validation scenarios
  • Includes tests for valid and invalid configurations
  • Tests for missing config/schema files

Documentation

  • Added documentation for the new dip validate command in README.md
  • Included example configuration with schema reference
  • Added detailed schema documentation in schema.json

Dependencies

  • Added json-schema (~> 5) as a new dependency
  • Included schema.json in gem package files

Checklist

  • I have added tests
  • I have made corresponding changes to the documentation

@bibendi
Copy link
Owner

bibendi commented Nov 15, 2024

Hey @oleander 👋🏻
Thanks for the PR! I'll take a look at it within the next few days.
I'm a bit concerned about the two new runtime dependencies. Without Bundler, this could be problematic, but I'm also hesitant to introduce Bundler. I'll check how other popular Ruby CLI tools usually handle this.

@oleander oleander mentioned this pull request Nov 15, 2024
lib/dip/config.rb Show resolved Hide resolved
lib/dip/config.rb Show resolved Hide resolved
lib/dip/config.rb Show resolved Hide resolved
@oleander oleander requested a review from bibendi November 19, 2024 15:07
Copy link
Owner

@bibendi bibendi left a comment

Choose a reason for hiding this comment

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

Alright, the PR looks very good to me! Thank you for the contribution! I need to fix the CI on Ruby 2.7. I'll do it this week.

@oleander
Copy link
Contributor Author

Great, thanks! I'll close the other PR.

@bibendi
Copy link
Owner

bibendi commented Nov 24, 2024

That's what I was afraid of. Rubygems cannot correctly install the transitive dependency public_suffix because it does not check the Ruby version as Bundler does. I suggest limiting the public_suffix version in the gemspec.

https://github.com/bibendi/dip/actions/runs/11915832181/job/33438788176

@oleander
Copy link
Contributor Author

I've limited the versions for public_suffix. Could you run the CI please?

@oleander oleander requested a review from bibendi November 25, 2024 09:30
@oleander
Copy link
Contributor Author

I ran the CI locally and had to make some adjustments to the versioning. The Ruby 3 constraint was introduced in version 6 of public_suffix, not 7. I updated the range accordingly.

@bibendi bibendi merged commit ab7cc37 into bibendi:master Nov 25, 2024
5 checks passed
@bibendi
Copy link
Owner

bibendi commented Nov 25, 2024

Released v8.2.0
Thank you!

@YOU54F
Copy link

YOU54F commented Nov 25, 2024

👋🏾 Hey all

This change has caused issues with the dip config in sbmt-pact

https://github.com/Kuper-Tech/sbmt-pact/blob/14255f3d6719af071082edb21312c19e65ef00e6/dip.yml#L38-L43

ERROR: Schema validation failed: The property '#/interaction/rspec/subcommands' contains additional properties ["rails-6.0", "rails-6.1", "rails-7.0"] outside of the schema when none are allowed

It is now affected as sbmt-pact does not pin the version of dip in the CI config

https://github.com/Kuper-Tech/sbmt-pact/blob/14255f3d6719af071082edb21312c19e65ef00e6/.github/workflows/tests.yml#L47

Not sure if the schema validator should be allowing those cases, or if the dip config needs to be updated

@bibendi
Copy link
Owner

bibendi commented Nov 26, 2024

Hey Yousaf!
Thank you for the reporting!
We will fix this today. You can temporary disable the validation with export DIP_SKIP_VALIDATION=true

@bibendi
Copy link
Owner

bibendi commented Nov 26, 2024

Fixed at v8.2.1

@bibendi bibendi mentioned this pull request Nov 26, 2024
6 tasks
@YOU54F
Copy link

YOU54F commented Nov 26, 2024

excellent, cheers Misha! - thanks for the extra tip re: skipping validation. (might be nice to have that in the error message, just in case anyone in the future needs to get out of a bind)

@ericpridham
Copy link

Hello! Is there a particular reason that this schema doesn't support array env values?

Validation failed: Schema validation failed: The property '#/environment/REPOS' of type array did not match one or more of the following types: string, boolean, number

If not, can support be added?

Thanks!

@bibendi
Copy link
Owner

bibendi commented Dec 4, 2024

Hi! Why do you need array env variables? After all, in the end, the value turns into a string anyway. And for an array, such a conversion does not look unambiguous.

@ericpridham
Copy link

@bibendi Thanks for the reply. A developer on my team pointed this out to me as well; when we use the value it's coming through as a string, so I suppose we were just using it as syntactic sugar. And all we need to do is add a | to make the validator happy, so no major rework needed.

By the way, this is a great tool and we appreciate all your work.

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.

4 participants