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_file is a no-op because valico isn't compatible with serde 1.0 #11

Open
emk opened this issue Dec 15, 2017 · 4 comments
Open

Comments

@emk
Copy link
Owner

emk commented Dec 15, 2017

The validate_file function in compose_yml is supposed to make sure that all the files we read and write correspond to an appropriate JSON schema. Unfortunately, this relies on valico, which appears to be currently unmaintained, and which only works with serde 0.8.

See s-panferov/valico#27 for a PR that would fix this.

@emk emk mentioned this issue Dec 15, 2017
emk added a commit that referenced this issue Dec 15, 2017
Notes:

- `valico` is not currently usable without extra hacks.  See
  #11 and
  s-panferov/valico#27
- `regex` has some API changes.
- `serde` has massive API changes.
- `error-chain` isn't worth the trouble of updating; we'll just switch
  over to `failure` sometime soon.

CC @seamusabshere @camjackson
@emk
Copy link
Owner Author

emk commented Dec 17, 2017

@camjackson We can certainly just disable this warning.

The lack of valico means that we probably have worse error messages, and we don't validate inputs and outputs as well as we should.

The real fix is to get a version of valico with the fixes mentioned above merged (and ditto for a second crate used by valico). At this point, that probably means forking both libraries and releasing them to crates.io.

There's also a trick we could do by releasing a single shim library that re-exports the older serde APIs (And serde JSON?) under a new name.

Probably the first step would be to ping upstream more aggressively.

But in the meantime, we can turn this warning off or replace it by debug! if that helps. Thoughts?

@camjackson
Copy link
Contributor

This is the nature of open source I guess!

As an end user, I think it would be best if Cage didn't log the extra information. It's quite noisy to have it there on every cage command, especially multiple times per command. But I also see the value in having something there in the code as a reminder that the validation function is a noop.

Would a debug! be silent when compiled into Cage? If so, that seems like a good idea to me. Or if debug! will still show up in the console, then maybe a comment would be better.

@emk
Copy link
Owner Author

emk commented Dec 17, 2017

A debug! would be silent unless logging were manually elevated. Or, actually, we could look at this line here, and add another rule for compose_yml::v2::validate, setting it to log::LogLevelFilter::Error.

On the other hand, this is actually a real compose_yml and cage regression (and not a particularly small regression!) and the best action would be to fix it ASAP.

So, choices:

  1. Convince upstream to merge Update serde to 1.x.y s-panferov/valico#27 and incorporate korczis/jsonway@8a142a0.
  2. less good Fork both libraries, rename them, and release them to crates.io. I'd prefer to avoid this. The above PRs have only been waiting for a month. (However, the underlying libraries haven't been updated in a year or so.)
  3. Build a shim library around serde_json which exports the old APIs under a different namespace, so that two different versions of serde_json can be used internally by compose_yml without conflict. Rust can actually handle this.

But the problem with (3) is that once we start releasing shim crates, why not just fork the crates we actually need and update them?

I can't find any other JSON Schema implementations in Rust at the moment. valico is the best game in town, AFAICT.

@camjackson
Copy link
Contributor

That all makes sense. Maybe let's put this on hold for a short while, giving upstream developers a chance to respond. And if they don't have bandwidth then we can consider the options further.

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

No branches or pull requests

2 participants