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

Replace json schema with go validation #781

Merged
merged 40 commits into from
Mar 12, 2019
Merged

Conversation

antho1404
Copy link
Member

Remove the json schema to replace it with a validation in go with https://github.com/go-playground/validator

It will be easier to plug some specific validation if we want too and it remove some extra complexity.

service/importer/definition.go Outdated Show resolved Hide resolved
service/importer/definition.go Outdated Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
Copy link
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

Should we only allow ascii in fields like description?

UTF will be better, but I can't find validator for this, we need to create custom validator for utf

service/importer/definition.go Outdated Show resolved Hide resolved
service/importer/definition.go Show resolved Hide resolved
service/importer/definition.go Outdated Show resolved Hide resolved
service/importer/service_file_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

As @ilgooz pointed out in #782

require at least one of the events or tasks sections

@antho1404
Copy link
Member Author

antho1404 commented Feb 11, 2019

require at least one of the events or tasks sections

Is it really necessary, I agree that it would be almost required but it's not something totally useless to have services without any tasks/event. We could have a service that basically just have one or multiple dependencies like a docker-compose. Maybe we can keep it simple for now, I'm open to both solutions

@krhubert
Copy link
Contributor

The question is - if someone will need a service without event or task? what will be usecase for service which you can't communicate with? If user has such use case then we even must allow both events and tasks to be empty. If there is no use case then importer should validate it and in the future we can take off this restriction if we find usecase for service without events or tasks.

@antho1404
Copy link
Member Author

I just worked on an IPFS this weekend that runs in a MESG service but that actually didn't expose anything except the dependency port to the IPFS node. I agree that this is not the best use of MESG (and I'm actually trying to merge it in the core directly) but this is a use case, just accessing a server from a dependency.

But again both are fine to me

@krhubert
Copy link
Contributor

If there is a use-case (event as you describe not the best) then we should allow such configuration 💪

@krhubert
Copy link
Contributor

Also it will close #695

@krhubert
Copy link
Contributor

krhubert commented Feb 14, 2019

I will replace gopkg.in/go-playground/validator.v9 with github.com/asaskevich/govalidator

motiviationŁ

  • there is one big issue with validator.v9 - it dosen't export functions to validate so we can only use it inside structs. On the other side govalidator exposes all functions and we are using it in commands/provider and we might want use this in the future.

EDIT:
As I look on both packages - validtor.v9 has better validation options (although it doesn't export any method for validation) and it's developed. As I removed govalidator.

NicolasMahe
NicolasMahe previously approved these changes Mar 11, 2019
@ilgooz
Copy link
Contributor

ilgooz commented Mar 11, 2019

@NicolasMahe have you tested each rule manually? Or I need to do that because we don't have full test cov. Code seems fine.

@NicolasMahe
Copy link
Member

NicolasMahe commented Mar 11, 2019

@NicolasMahe have you tested each rule manually? Or I need to do that because we don't have tests. Code seems fine.

I tested a lot manually.

@ilgooz
Copy link
Contributor

ilgooz commented Mar 11, 2019

@NicolasMahe have you tested each rule manually? Or I need to do that because we don't have tests. Code seems fine.

I tested a lot manually.

Ok, let me do one more in case if we miss something and then I'll merge.

ilgooz
ilgooz previously approved these changes Mar 11, 2019
@ilgooz
Copy link
Contributor

ilgooz commented Mar 11, 2019

@NicolasMahe I cannot merge because some commits are not signed.

@NicolasMahe NicolasMahe dismissed stale reviews from ilgooz and themself via f14ba8e March 12, 2019 05:26
@NicolasMahe
Copy link
Member

i fixed the optional SID and added some test

please review

NicolasMahe
NicolasMahe previously approved these changes Mar 12, 2019
krhubert
krhubert previously approved these changes Mar 12, 2019
@NicolasMahe NicolasMahe dismissed stale reviews from krhubert and themself via bcf417b March 12, 2019 09:55
@NicolasMahe NicolasMahe merged commit b215cc4 into dev Mar 12, 2019
@NicolasMahe NicolasMahe deleted the feature/go-validation branch March 12, 2019 12:01
@antho1404
Copy link
Member Author

finished my tests and everything is fine :)

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