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

yaml support #4

Closed
zendril opened this issue Mar 20, 2020 · 6 comments
Closed

yaml support #4

zendril opened this issue Mar 20, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@zendril
Copy link

zendril commented Mar 20, 2020

It doesn't appear that it works with yaml schema or target file.
I know that the underlying xeipuuv/gojsonschema will support that as they are using it in Helm for validating the values.yaml.

Can this support be added?

@neilpa neilpa added the enhancement New feature or request label Mar 20, 2020
@neilpa
Copy link
Owner

neilpa commented Mar 20, 2020

I know that the underlying xeipuuv/gojsonschema will support that

That doesn't appear to be the case. I don't see any yaml parsers referenced in the project's go.mod file

they are using it in Helm for validating the values.yaml.

Looks like Helm first parses the yaml file to a map[string]interface{} and then converts back to json before validating.

Can this support be added?

I'm not sure. This tool is a simple wrapper around the xeipuuv/gojsonschema and I'd like to keep it as small as possible.

I'd be open to a reviewing a PR that adds yaml support, but it should be explicitly differentiated on the command line, e.g. yajsv -s schema.json -y data.yaml data.json. This looks like a potentially good library if you're interested in doing this.

Depending on the complexity though, I may still decline it as there plenty of tools for doing yaml-to-json conversion out of band.

@zendril
Copy link
Author

zendril commented Mar 21, 2020

Heya.. was popping back in to comment. I have it working. It is a super lightweight change and no change to the user interactions.

I have it look for .yaml or .yml in either the schema or the target file. If found, convert and then load bytes, if not, do your current logic.

I do have one question, why are you sometimes converting to putting file:// at the beginning. If I'm reading the comment/code correctly it looks like you did this for windows support, but I'm on windows and that seems to break.

I'll create a PR out of what I have now just so you can take a quick look and opine on it, but it very much 'hacked in' and will need some cleanup/refactoring.

@zendril
Copy link
Author

zendril commented Mar 21, 2020

I pulled your code to merge into mine before doing a PR, it looks like you added an import that isn't accessible:

https fetch failed: Get https://neilpa.me/go-x?go-get=1: Forbidden

Can you take a look/fix?

I also see that you have gox in the makefile. If this is the library you are trying to pull in, then I might have a different way of doing a comprehensive makefile w/o needing that.

@neilpa
Copy link
Owner

neilpa commented Mar 21, 2020

I have it working. It is a super lightweight change and no change to the user interactions.

Great! Looking forward to the PR.

I have it look for .yaml or .yml in either the schema or the target file. If found, convert and then load bytes, if not, do your current logic.

Yea, after thinking about it some more that seems like a better approach than adding new command line flags for yaml vs json.

I do have one question, why are you sometimes converting to putting file:// at the beginning.

This is because the xeipuuv/gojsonschema package expects URIs to resolve both schemas and documents. That allows it to support both local file:// and remote http:// resources. For this CLI we only care about local paths and have to do the conversion to URIs.

If I'm reading the comment/code correctly it looks like you did this for windows support

Yea, my original implementation was super naive, e.g. uri = "file:// + filepath.Abs(path)" which doesn't work on Windows. There was a basic fix in #2 to deal with '\' and spaces. I updated that in #5 to use a wrapper around the win32 api to handle the conversion.

@neilpa
Copy link
Owner

neilpa commented Mar 21, 2020

https fetch failed: Get https://neilpa.me/go-x?go-get=1: Forbidden

Hmm, I thought I made everything thing public when I did those changes yesterday. I'll see if I can figure out what I missed.

I also see that you have gox in the makefile. If this is the library you are trying to pull in, then I might have a different way of doing a comprehensive makefile w/o needing that.

I added gox as a quick way to make the pre-built binaries I was attaching to releases. I'm fine removing that dependency and doing it purely in the Makefile with GOOS and GOARCH. Especially now that I've had a few other people start using this tool besides myself.

@zendril
Copy link
Author

zendril commented Mar 22, 2020

👏

@zendril zendril closed this as completed Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants