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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a new JSON schema generation engine (Beta feature) #980

Merged
merged 40 commits into from Sep 12, 2019

Conversation

@sladyn98
Copy link
Contributor

sladyn98 commented Aug 7, 2019

Action Items Covered in this PR:
a) JSON Schema Generation Covered for the following configurators:

     1 ) Root Configurators
     2) HeteroDescribable Configurators
     3) Base Configurators

b) Unit Testing for the schema.

Your checklist for this pull request

馃毃 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Aug 7, 2019

Added an initial test to check if the page responds.
cc @timja @casz @oleg-nenashev

WebResponse response = client.loadWebResponse(request);
assertThat(response.getStatusCode(), is(200));
String schema = response.getContentAsString();
String contents = new String(Files.readAllBytes(Paths.get("./src/test/resources/io/jenkins/plugins/casc/SchemaTest.yml")));

This comment has been minimized.

Copy link
@timja

timja Aug 8, 2019

Member

you should be able to get this as a resource,
something like:

Suggested change
String contents = new String(Files.readAllBytes(Paths.get("./src/test/resources/io/jenkins/plugins/casc/SchemaTest.yml")));
String contents = new String(Files.readAllBytes(getClass().getResource("SchemaTest.yml")));

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 9, 2019

Author Contributor

String contents = new String(Files.readAllBytes(Paths.get(getClass().getResource("SchemaTest.yml").toURI())));
Cool, somtheing like this works.

This comment has been minimized.

Copy link
@casz

casz Aug 9, 2019

Member

Hmmm we already have a helper function inside the util package

This comment has been minimized.

Copy link
@casz

casz Aug 9, 2019

Member

public static String toStringFromYamlFile(Object clazz, String resourcePath) throws URISyntaxException, IOException {

plugin/pom.xml Outdated Show resolved Hide resolved
configurationAsCode.getConfigurators()
.stream()
.forEach(configurator -> {
if(configurator.getClass().getSimpleName().equals("HeteroDescribableConfigurator")) {

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 17, 2019

Author Contributor

@timja Probably we could do this for every object :) If there is a better way do suggest it

This comment has been minimized.

Copy link
@timja

timja Aug 17, 2019

Member

something like:

Suggested change
if(configurator.getClass().getSimpleName().equals("HeteroDescribableConfigurator")) {
if (configurator instanceof HeteroDescribableConfigurator.class)) {

eventually we would like to lookup the schema generation by the configurator type, but not needed yet, that would allow other people to contribute schema generators from a custom configurator

pom.xml Outdated Show resolved Hide resolved

} else if (configurator instanceof JenkinsConfigurator) {
JenkinsConfigurator jenkinsConfigurator = (JenkinsConfigurator) configurator;
descriptorExtensionList = j.jenkins.getDescriptorList(jenkinsConfigurator.getTarget());

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 20, 2019

Author Contributor

@timja for eg here the jenkins object does not have a target method that returns the type we want for the getDescriptorList function

@casz

This comment has been minimized.

Copy link
Member

casz commented Aug 21, 2019

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Aug 22, 2019

@casz That is what we are trying to fix.The getDescritptorList needs a param that some of the confgiurators are unable to provide.eg unsecuredAuthorizationStrategyConfigurator.getTarget().I am debugging it at the moment, any ideas would do..:)

@@ -253,7 +253,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.17</version>
<version>1.19</version>

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 22, 2019

Author Contributor

@casz This was the change made

This comment has been minimized.

Copy link
@casz

casz Aug 22, 2019

Member

I have no issue with running mvn install inside IDEA did you remember to either import POM changes or enable auto update based on changes inside the POM?

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 22, 2019

Author Contributor

I refresh the dependencies everytime I make a change, so I think I should be fine.

plugin/pom.xml Outdated Show resolved Hide resolved
plugin/pom.xml Outdated Show resolved Hide resolved
@sladyn98 sladyn98 added the dev-tools label Aug 22, 2019
@sladyn98 sladyn98 self-assigned this Aug 22, 2019
System.out.println("Look at the beautiful Json data");
System.out.println(output);
try {
String indented = (new JSONObject(output)).toString(4);

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 23, 2019

Author Contributor

@casz @timja used this for the beautification of the string

* The initial template for the JSON Schema
*/
StringBuilder schemaString = new StringBuilder();
schemaString.append("{\n" +

This comment has been minimized.

Copy link
@timja

timja Aug 23, 2019

Member

this should either be in a constant or a resource file

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 23, 2019

Author Contributor

Yeah I will add a file called schema there :)

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 24, 2019

Author Contributor

@casz I could save the resources path in a static string and then save the JSONSchema in the resources folder .That could be picked up from the test file as well.What do you think ?
Since using getClass().getResource("/").getPath() is proving a bit of a pain 馃槥

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

Make it a constant?

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 24, 2019

Author Contributor

Rather we would anyway need to store the schema in a resource folder.What do you think of writing the schema into a Schema.json in a resource folder.

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

The schema will be different for each Jenkins master, so we will never store a schema.json

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 24, 2019

Author Contributor

But for tests wont we need the schema to validate against a yaml file ? Wont we need to have a schema to validate against.

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

We will generate schema in a given test and then we could have our test validate against all of our existing .yml files in the resources folder yes.

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

Again mixing things here. @timja initial comment was that the very static

{
"$schema": "http://json-schema.org/draft-06/schema#",
"id": "http://jenkins.io/configuration-as-code#",
"description": "Jenkins Configuration as Code",
"type": "object"
}

Should be kept in a constant or a file. For simplicity, I suggest it is stored as a constant of type JSONObject.

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 24, 2019

Author Contributor

Yeah @casz thanks for the suggestion I got that :)
I just wanted your feedback on storing it in the test hence the different question.

}

@Test
public void checkRootConfigurators() {

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 23, 2019

Author Contributor

@casz @oleg-nenashev would it be a good thing if we check the schema against individual configurators.
Considering we are using two configurators
a) RootConfigurators
b) BaseConfigurators
c) HeteroDescribableConfigurators.

@@ -0,0 +1,156 @@
package io.jenkins.plugins.casc;

import java.util.*;

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

Your using IntelliJ right?

I wonder how you have configured your project, it seems to ignore the .idea/codeStyles

/**
* The initial template for the JSON Schema
*/
StringBuilder schemaString = new StringBuilder();

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

I think with we switch to using JSON builder seems tedious to use strings.

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Aug 24, 2019

Author Contributor

JSON builder might refuse the trailing commas

This comment has been minimized.

Copy link
@casz

casz Aug 24, 2019

Member

When using a JSON builder you should not be concerned about the delimiters you should be using JSON objects and JSON arrays to build out the schema.
Frees you from thinking how to write JSON in a string format.

/**
* The initial template for the JSON Schema
*/
JSONObject schemaObject = new JSONObject(schemaTemplateObject.toString());

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 7, 2019

Author Contributor

@timja I'm kind of sure that Intellij asked me to import org.everit.json while constructing JSONObject

This comment has been minimized.

Copy link
@timja

timja Sep 7, 2019

Member

It doesn鈥檛 appear to be used though?

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 7, 2019

Author Contributor

It does becasue if we remove it JSONObject then requires an import

sladyn98 added 2 commits Sep 7, 2019
@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 7, 2019

Codes looking good, I鈥檒l try do some testing tomorrow / the next day and report back

* @throws Exception
*/
@RequirePOST
public void dov2Schema(StaplerRequest req, StaplerResponse res) throws Exception {

This comment has been minimized.

Copy link
@timja

timja Sep 8, 2019

Member
  1. this isn't linked to from anywhere, is that intentional?

  2. does this need to be require post? from a rest api design it doesn't really make sense and the previous schema was available as a GET
    cc @oleg-nenashev

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 9, 2019

Author Contributor

I Tried using Postman and it still does not work :(

This comment has been minimized.

Copy link
@timja

timja Sep 9, 2019

Member

it works for me, http://localhost:8080/jenkins/configuration-as-code/v2Schema

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 10, 2019

Author Contributor

Thats great then, we are almost done with this PR

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 11, 2019

Author Contributor
  1. this isn't linked to from anywhere, is that intentional?
  2. does this need to be require post? from a rest api design it doesn't really make sense and the previous schema was available as a GET
    cc @oleg-nenashev

@timja We could make it a GET

@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 8, 2019

@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 8, 2019

Also it doesn't seem to suggest values to me which I expected it to do and doesn't care if I put values it doesn't know about in

It does catch invalid type though

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Sep 8, 2019

It does catch invalid type though

Yeah this is actually what it is programmed to do. Catch invalid types

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Sep 8, 2019

Also it doesn't seem to suggest values to me which I expected it to do and doesn't care if I put values it doesn't know about in

For this to be succesfully we would need the nested structure of all of the configurators, with each configurator describing itself recursively, the way its currently designed all of the nesting is in base configurator, so the schema isnt really nested, and thus it doesnt know what fields are coming ahead and thus doesnt care.

@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 8, 2019

Also it doesn't seem to suggest values to me which I expected it to do and doesn't care if I put values it doesn't know about in

For this to be succesfully we would need the nested structure of all of the configurators, with each configurator describing itself recursively, the way its currently designed all of the nesting is in base configurator, so the schema isnt really nested, and thus it doesnt know what fields are coming ahead and thus doesnt care.

Ok I think we will need that but we can do that as part of a separate issue

@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 11, 2019

@sladyn98 we should be able to get this merged if we can get this linked to from the configuration-as-code page instead of the existing schema.

@timja timja requested a review from casz Sep 11, 2019
@@ -54,7 +54,7 @@
<h2>${%Reference}</h2>
<dt>
<dl><a href="reference">${%Documentation}</a></dl>
<dl><a href="schema">${%JSON schema}</a></dl>
<dl><a href="v2Schema">${%JSON schema}</a></dl>

This comment has been minimized.

Copy link
@timja

timja Sep 11, 2019

Member

won't that send the user to a page which says they need to click a button to send it as a post request?

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 11, 2019

Author Contributor

Yeah it would because the method says REQUIRE POST

This comment has been minimized.

Copy link
@timja

timja Sep 11, 2019

Member

is that required? cc @casz and @oleg-nenashev

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 11, 2019

Member

GET is fine here. As long as the call dies not modify the instance state, get requests are fine

@timja
timja approved these changes Sep 12, 2019
@casz
casz approved these changes Sep 12, 2019
@timja timja merged commit 73f5959 into jenkinsci:master Sep 12, 2019
5 checks passed
5 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: Java No new or fixed alerts
Details
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oleg-nenashev oleg-nenashev changed the title JCasC JSONSchema Generation and Testing Introduce a new JSON schema generation engine (Beta) Sep 12, 2019
@oleg-nenashev oleg-nenashev changed the title Introduce a new JSON schema generation engine (Beta) Introduce a new JSON schema generation engine (Beta feature) Sep 12, 2019
@sladyn98 sladyn98 referenced this pull request Sep 17, 2019
0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can鈥檛 perform that action at this time.