-
Notifications
You must be signed in to change notification settings - Fork 41
add local testing of json schema generation #316
Conversation
Tested it and it works |
@ashetty1 thanks! |
@kadel ping |
Makefile
Outdated
@@ -86,7 +86,7 @@ endif | |||
|
|||
# Run all tests | |||
.PHONY: test | |||
test: test-dep validate test-unit | |||
test: test-dep validate test-unit test-jsonschema-generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should not be included.
This suddenly uses a Docker container.
Similar to how we have integration tests / unit tests / cmd tests separated into their separate commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not safe to assume that the kedge developers have docker installed locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it's not safe, and tests should be ran separately... testing json-schema-generation should be separate just like test-unit, test-ci, test-cmd, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done changes as requested!
b61a9d2
to
946fe79
Compare
if I run it locally, I get following error:
|
using |
946fe79
to
d8be756
Compare
for some reason curl seemed to fail in there
yes added sudo in front of docker command and also added sudo: required in travis config file |
Makefile
Outdated
# Test if the changed spec.go is valid and JSONSchema can be generated out of it | ||
.PHONY: test-jsonschema-generation | ||
test-jsonschema-generation: | ||
sudo docker run -v `pwd`/pkg/spec/spec.go:/data/spec.go:ro,Z surajd/kedgeschema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having anything "sudo" within Makefile is really bad...
Please remove sudo! If a user wishes to run test-jsonschema-generation they can use make test-jsonschema-generation
see other comment, after adding:
sudo: required
services:
- docker
you wont need to sudo the docker command anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved!
@@ -1,5 +1,7 @@ | |||
dist: trusty | |||
|
|||
sudo: required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need to add docker as a service to here too:
sudo: required
services:
- docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
d8be756
to
16d57d7
Compare
Other than SemaphoreCI test failing, LGTM! |
No description provided.