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

Validate deploy.yaml with json-schema #37

Closed
wants to merge 17 commits into from

Conversation

kjellberg
Copy link
Contributor

@kjellberg kjellberg commented Feb 4, 2023

A suggestion to #6 - catch errors in deploy.yaml better.

lib/configuration/schema.yaml sets json-schema rules for deploy.yaml.

Must be extended with validations for all available options, just want to hear your opinion first.

Example 1

# config/deploy.yaml
service: my-app
image: user/my-app
servers:
  - 192.168.0.1
registry:
  password: my-password-should-go-somewhere-safe
$ mrsk details
  ERROR (Mrsk::Configuration::ConfigurationError): The property '#/registry' did not contain a required property of 'username'

Example 2

# config/deploy.yaml
service: 123
image: user/my-app
servers:
  - 192.168.0.1
registry:
  username: my-user
  password: my-password-should-go-somewhere-safe
$ mrsk build create
  ERROR (Mrsk::Configuration::ConfigurationError): The property '#/service' of type integer did not match the following type: string

@seomamma
Copy link

seomamma commented Feb 4, 2023

I like where this is going.

@kjellberg kjellberg force-pushed the validate-with-json-schema branch 2 times, most recently from 41e2bcb to 6a7fabd Compare February 6, 2023 10:48
@dhh
Copy link
Member

dhh commented Feb 9, 2023

I dig this. The error messages aren't super friendly out the box, but it's better than what we have now: which is half baked! Please do continue 👍

Gemfile Outdated Show resolved Hide resolved
@kjellberg kjellberg marked this pull request as draft February 23, 2023 10:59
@kjellberg kjellberg marked this pull request as ready for review March 3, 2023 22:19
@kjellberg
Copy link
Contributor Author

I'd appreciate some feedback here. I think I've covered all configuration options but there may be cases I've missed. For example I just discovered in one of my projects that env/clear/VAR didn't accept booleans (fixed now).

Therefore, I'd appreciate if you could try to run my patch with your current working configurations and tell me how it works.

It passes all tests, test fixtures and the examples in README.md. It doesn't break if new config options are introduced, just that they wont be validated until added in the schema.

So, I think this is ready to merge. Would be nice to have the schema merged into the repo, so we can keep it synced with new features and patches. I'm ready to assist should errors occur.

@dhh
Copy link
Member

dhh commented Mar 9, 2023

Awesome. Will check it out now. Yes, we just had an issue this morning at 37s because of lacking schema validation 😄

@dhh
Copy link
Member

dhh commented Mar 20, 2023

What can we do to improve the error messages? They're really quite rough right now. Like if I change "servers" to "server", I get: ERROR (Mrsk::Configuration::Error): The property '#/' did not contain a required property of 'servers'.

@dhh
Copy link
Member

dhh commented Mar 20, 2023

If I change builder/args to builder/arguments, it doesn't complain. Is there a way to make a list of properties exhaustive?

@dhh
Copy link
Member

dhh commented Mar 20, 2023

Think we might need some more exhaustive testing.

@huksley
Copy link
Contributor

huksley commented Apr 10, 2023

Note to users: While you are waiting for this and using VS Code,
add this as the first line of the deploy.yml file:

# yaml-language-server: $schema=https://raw.githubusercontent.com/kjellberg/mrsk/validate-with-json-schema/lib/mrsk/configuration/schema.yaml

@sharkovadarya
Copy link

Hello! Are there currently any plans for a JSON schema for Kamal, whether from this PR or any other source, to be available in Rails 8.0 or any subsequent releases? I see there hasn't been any activity for quite some time.

@djmb
Copy link
Collaborator

djmb commented Jun 19, 2024

Superseded by #828

@djmb djmb closed this Jun 19, 2024
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.

None yet

9 participants