-
Notifications
You must be signed in to change notification settings - Fork 724
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
Schema Generation for nested yml configurations #1027
Conversation
Bumped up schema version to v07. Added id instead of ref Added basic schema validation tests.
.../src/main/java/io/jenkins/plugins/casc/impl/configurators/HeteroDescribableConfigurator.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/jenkins/plugins/casc/impl/configurators/HeteroDescribableConfigurator.java
Outdated
Show resolved
Hide resolved
521d972
to
13e893a
Compare
.../src/main/java/io/jenkins/plugins/casc/impl/configurators/HeteroDescribableConfigurator.java
Outdated
Show resolved
Hide resolved
WDYT about #1027 (comment) @timja ? |
/** | ||
* This function is for the JSONSchemaGeneration | ||
* @param instance Owner Instance | ||
* @param context Context to be passed | ||
* @return CNode object describing the structure of the node | ||
*/ | ||
public CNode describeStructure(Owner instance, ConfigurationContext context) { | ||
public CNode describeForSchema(Owner instance, ConfigurationContext context) { |
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.
DescribeStructure would then no longer be needed as the describeForSchema would essentially perform the check.
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.
Do we still need the mode var?
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.
We would not need it in the describeForSchema
anymore because if isJsonSchema
is set then it would anyways be calling describeStructure
which would in turn call describeForSchema
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.
ok please remove then
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:+1:
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.
Few questions on whether some of the code is still required and a little bit of cleanup
@@ -111,6 +112,10 @@ boolean isIgnored() { | |||
return restrictions != null ? restrictions : EMPTY; | |||
} | |||
|
|||
public void setJsonSchema(boolean jsonSchema) { |
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 this still required?
@@ -275,7 +323,12 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi | |||
*/ | |||
private CNode _describe(Configurator c, ConfigurationContext context, Object value, boolean shouldBeMasked) | |||
throws Exception { | |||
CNode node = c.describe(value, context); | |||
CNode node; | |||
if(isJsonSchema) { |
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 this still required?
@oleg-nenashev / @Casz anyone available to give this a (hopefully) final review? |
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.
I do not see obvious implementation issues here. My suggestion would be to fix the tests, mark new API as '@restricted(Beta.class)' and release so that adoption can start
@@ -167,4 +168,20 @@ default CNode describe(T instance, ConfigurationContext context) throws Exceptio | |||
return mapping; | |||
} | |||
|
|||
@CheckForNull |
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.
Needs Javadoc since it will be consumed by plugins. @since TODO
also
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.
Yeah added javadoc. Confused about the @since TODO
?
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.
All comments look addressed I'll merge this later today if there's no other feedback
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.
Minor code nitpicking
Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
Not all nested elements are found: example "vSphereCloud": {
"additionalProperties": false,
"type": "object",
"properties": {"idleMinutes": {
"description": "",
"type": "integer"
}}
}, |
@Casz Yeah we tried to maximize nesting as much as possible, but it still doesn't work in all cases |
There’s lots of limitations... this is a big improvement over what was previously there and it can be incrementally improved |
* Add support to backup and restore automatically * Fix the potential output resource leak * Add configs about deploy into alauda update-center * Set alauda as the main branch * Add sonarqube properties file * Fix the potentital issues * Add a merge strategy extension point for the YAML config (#1) * Add YAML merge strategy * Add a new merge strategy * Remove override merge strategy * Clean the unused code lines * Clean the unused imports * Add test cases for merge strategy * Fix the wrong test cases * Take merge strategy name everytime * Give the default strategy a more readable name * Validate top README with the Integrations tests (jenkinsci#1229) * Fix the compilation error caused by merge strategy * Fix issues which found by spotbugs * Remove unused imports * Move profiles from pom.xml into settings file * Remove empty line * rename the shared-library * Schema Generation for nested yml configurations (jenkinsci#1027) * Trying to diagnose some flaky tests (jenkinsci#1243) * Update ci to include code coverage after jenkinsci#1215 (jenkinsci#1241) Co-authored-by: Oleg Nenashev <o.v.nenashev@gmail.com> * Checkstyle ignore release.properties (jenkinsci#1245) * [maven-release-plugin] prepare release configuration-as-code-1.35 * [maven-release-plugin] prepare for next development iteration * Bump jackson.version from 2.10.1 to 2.10.2 Bumps `jackson.version` from 2.10.1 to 2.10.2. Updates `jackson-databind` from 2.10.1 to 2.10.2 - [Release notes](https://github.com/FasterXML/jackson/releases) - [Commits](https://github.com/FasterXML/jackson/commits) Updates `jackson-dataformat-yaml` from 2.10.1 to 2.10.2 - [Release notes](https://github.com/FasterXML/jackson-dataformats-text/releases) - [Commits](FasterXML/jackson-dataformats-text@jackson-dataformats-text-2.10.1...jackson-dataformats-text-2.10.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * Correct spelling of hpi.pluginChangelogUrl tag (jenkinsci#1248) * Automatically request code reviews from all JCasC plugin developers (jenkinsci#1249) * [Security] Bump checkstyle from 8.26 to 8.29 Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 8.26 to 8.29. **This update includes a security fix.** - [Release notes](https://github.com/checkstyle/checkstyle/releases) - [Commits](checkstyle/checkstyle@checkstyle-8.26...checkstyle-8.29) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * It has been renamed to `master.componentName` (jenkinsci#1256) Co-authored-by: Tim Jacomb <t.jacomb@kainos.com> * Fix the version specification for Jenkins 2.199 (jenkinsci#1268) * Added example for credentials certificate (jenkinsci#1270) * Use the latest shared-library branch * Update test to match latest plugin version (jenkinsci#1273) * Update the integration test for active-directory (jenkinsci#1274) * Add integration test for pipeline-model-definition (jenkinsci#1276) * Fix the error when no casc user dir (#5) * Add missing of mode in the context * Use the same version of jackson-core * Use the same version of jackson-core * Use 2.8 as the major version * Fix the wrong parent id for the plugin releasing * Move the user config into a backup dir * Run build under java pod Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com> Co-authored-by: Sladyn <sladynnunes98@gmail.com> Co-authored-by: Jesse Glick <jglick@cloudbees.com> Co-authored-by: Joseph Petersen <josephp90@gmail.com> Co-authored-by: Oleg Nenashev <o.v.nenashev@gmail.com> Co-authored-by: Tim Jacomb <t.jacomb@kainos.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Mark Waite <mark.earl.waite@gmail.com> Co-authored-by: Jhon Mike <jhon.msdev@gmail.com> Co-authored-by: Dawid Gosławski <50369002+dg-nvm@users.noreply.github.com> Co-authored-by: choeffer <33625292+choeffer@users.noreply.github.com> Co-authored-by: Francisco Fernández <31063239+fcojfernandez@users.noreply.github.com>
This PR deals with the schema generation for nested yml files.
We would be focusing on
a) Implementation of a
describeStructure
function for all the configurators which would then be used in the schema generation.b) Testing Framework for the Schema
Closes #1038
Closes #997
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.