Skip to content
This repository has been archived by the owner on Nov 6, 2018. It is now read-only.

Issue 229 validation #232

Merged
merged 8 commits into from
Nov 2, 2017
Merged

Conversation

hrishin
Copy link
Member

@hrishin hrishin commented Sep 25, 2017

JSON schema change for #229

  • added "resources" property to JSON schema tree

@iocanel
Copy link
Member

iocanel commented Sep 26, 2017

@jimmidyson: Can you please have a look?

@hrishin hrishin requested a review from iocanel September 27, 2017 05:31
@hrishin
Copy link
Member Author

hrishin commented Sep 27, 2017

@iocanel It would be great even if you can review it :)

@hrishin
Copy link
Member Author

hrishin commented Oct 3, 2017

@iocanel @jimmidyson could you please review it ASAP?

@@ -88,7 +88,9 @@ type Schema struct {
ImageList imageapi.ImageList
ImageStreamList imageapi.ImageStreamList
ImageStreamTagList imageapi.ImageStreamTagList
DeploymentConfig deployapi.DeploymentConfig
Copy link
Contributor

@jimmidyson jimmidyson Oct 3, 2017

Choose a reason for hiding this comment

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

Needs go fmting

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

DeploymentConfigList deployapi.DeploymentConfigList
Route routeapi.Route
Copy link
Contributor

Choose a reason for hiding this comment

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

go fmt

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -12600,6 +12600,10 @@
"$ref": "#/definitions/kubernetes_extensions_Deployment",
"javaType": "io.fabric8.kubernetes.api.model.extensions.Deployment"
},
"DeploymentConfig": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these lost in a previous generation? These have always been there before, no idea how they're newly added now?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact me I was surprised for this

@jimmidyson
Copy link
Contributor

The updated schema in this PR doesn't contain any resources elements, which I thought was the point of this PR? I'm a bit confused tbh. Plus my comment on the relevant issue about whether this is breaking a valid JSON schema document to add a custom element (resources) that my initial look can't find in the spec.

@hrishin hrishin force-pushed the issue-229-validation branch from 369b418 to 8293856 Compare October 4, 2017 05:27
@hrishin
Copy link
Member Author

hrishin commented Oct 4, 2017

@jimmidyson added updated JSON schema. It may not be valid as per the JSON schema specification, but I have tested this change with kubernetes-client, f8-m-p project and its not breaking anything yet.

@jimmidyson
Copy link
Contributor

I'm still not clear what this does for you to help with validation. Why is the JSON schema not good enough in its current form?

@hrishin
Copy link
Member Author

hrishin commented Oct 5, 2017

Hey @jimmidyson

I'm still not clear what this does for you to help with validation. Why is the JSON schema not good enough in its current form?

In f8-m-p we have enabled resource/spec/template validation with JSON schema. Please read fabric8io/fabric8-maven-plugin#1051 (comment) first point.

@hrishin hrishin force-pushed the issue-229-validation branch from 8293856 to 6c8f134 Compare October 10, 2017 10:02
@hrishin hrishin force-pushed the issue-229-validation branch from 6c8f134 to 772aaef Compare October 25, 2017 11:21
@hrishin
Copy link
Member Author

hrishin commented Oct 25, 2017

@jimmidyson @jstrachan
Created a new validation schema file validation-scehma.json for the f8-m-p validation only.
Could you please review it?

@jstrachan
Copy link
Contributor

LGTM

@hrishin
Copy link
Member Author

hrishin commented Oct 30, 2017

@jimmidyson @jstrachan @iocanel how to trigger the build again. Just wondering build is getting failed on this environment but getting passed on my dev. environment. Any suggestions?

@oscerd
Copy link
Member

oscerd commented Oct 30, 2017

@oscerd
Copy link
Member

oscerd commented Oct 30, 2017

retest this please

@rawlingsj
Copy link
Contributor

I added some JVM flags last week that were recommended which may be affecting this. Lemme remove and kick off another build to see if that helps.

@hrishin
Copy link
Member Author

hrishin commented Oct 30, 2017

@rawlingsj thanks a ton!

@hrishin hrishin force-pushed the issue-229-validation branch from e6bd1de to b9d12ef Compare November 2, 2017 09:04
@hrishin hrishin merged commit 049172a into fabric8io:master Nov 2, 2017
kadel pushed a commit to kadel/fabric8-pipeline-library that referenced this pull request Nov 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants