Validate YAML schedules against JSON Schema #10160

perlpunk · 2020-04-30T10:43:47Z

Issue: https://progress.opensuse.org/issues/66230

This makes sure that the schedule files contain the correct structure.

Otherwise it can happen that it contains things like:

schedule:
- {{foo}}

Which is valid YAML, but it's actually a YAML mapping, short for:

schedule:
- { { foo: null }: null }

and it must be quoted.
The JSON Schema validation will make sure it is always a string.

Please see the TODO item in t/schema/Schedule-1.yaml. Maybe we should make sure that every schedule file contains a description.

rwx788 · 2020-04-30T11:35:03Z

cpanfile

@@ -61,5 +61,5 @@ on 'test' => sub {
  requires 'Test::MockObject';
  requires 'Test::More', '0.88';
  requires 'Test::Warnings';
-  requires 'Test::YAML::Valid';
+  requires 'JSON::Validator';


Do not forget to run tools/update_spec to update spec file.

I did, but it did not change the spec file. Probably because it's only a test requirement.

rwx788

Looks very promising!

foursixnine · 2020-04-30T12:24:22Z

t/schema/Schedule-1.yaml

+additionalProperties: false
+required:
+# TODO Every file should have a description?
+#- description


Definitely, it should be required

That's weird, I commented it out because I got some files that didn't have a description.
Just tried it out again and now all files pass =)

Will remove the comment...

I guess not every schedule has it atm, and on top we have sometimes multiple schedules, where we duplicate description, but will use merge keys feature to avoid it in future. So I would not force description to be defined here, as there are multiple approaches in different teams. Some rely on openQA test suites, for example.

Hm, I removed a description from a file and tests still pass. Somethings wrong with the schema...

Stupid me.

I made a mistake in test_yaml_valid.
So we have around 60 files without a description field, so I suggest I keep that commented out for now.
Also there are 5 files that have a description: without a value.

@rwx788 do you have an example on where the description is duplicated?. My point on description being obligatory would be: There has to be a reason for that particular schedule to exist. But I guess that could be enabled later on.

PS: @perlpunk FSM is using it's tentacles to mess with JSON and YAML altogether! RUN!

@foursixnine For example https://github.com/os-autoinst/os-autoinst-distri-opensuse/tree/master/schedule/yast/lvm_raid1 as instead putting complex conditions in the schedule itself we sometimes create separate file for some exotic backend or setup. In this case we could have imported common description for all of them, as well as some settings.

Point taken :)

perlpunk · 2020-04-30T12:36:37Z

Any idea for a nice short description of what YAML schedules are, for the schema file?

rwx788 · 2020-04-30T12:41:09Z

Any idea for a nice short description of what YAML schedules are, for the schema file?

"YAML schedules are test suite definitions with data required for the tests and list of test modules to be executed."
This would be the best definition I can come up with right now =). Hope it helps.

foursixnine · 2020-04-30T12:54:50Z

"YAML schedules are test suite definitions with data required for the

Any idea for a nice short description of what YAML schedules are, for the schema file?

"YAML schedules are test suite definitions with data required for the tests and list of test modules to be executed."

YAML schedules define the list of test modules to be executed by a given job, it also allows setting variables that will affect the job to be executed in the end.

Issue: https://progress.opensuse.org/issues/66230 This makes sure that the schedule files contain the correct structure. Otherwise it can happen that it contains things like: schedule: - {{foo}} Which is valid YAML, but it's actually a YAML mapping, short for: schedule: - { { foo: null }: null } and it must be quoted. The JSON Schema validation will make sure it is always a string.

kalikiana · 2020-04-30T15:43:34Z

Makefile

-	$(eval YAMLS=$(shell sh -c "git --no-pager diff --diff-filter=dr --name-only master | grep '\(schedule\|test_data\)/.*\.y.\?ml$$'"))
-	if test -n "$(YAMLS)"; then \
-	  export PERL5LIB=${PERL5LIB_} ; tools/test_yaml_valid $(YAMLS);\
+	@# Get list of changed yaml files. We only want to lint changed files for


@# 🤨 Not something you introduced I guess but it seems odd to me to hide comments specifically here.

kalikiana · 2020-04-30T15:45:54Z

Makefile

-	if test -n "$(YAMLS)"; then \
-	  export PERL5LIB=${PERL5LIB_} ; tools/test_yaml_valid $(YAMLS);\
+	@# Get list of changed yaml files. We only want to lint changed files for
+	@# now because yamllint complains about a lot of existing files


Can you re-phrase that? Should that be a TODO? Is this about limiting the files you are fixing now?

Before only changed files were validated with test_yaml_valid and linted with yamllint.
Now we validate all files, but still we can't run yamllint on all files because there are too many errors to fix in this PR.
This should be done in a followup IMHO.
Any suggestion for rephrasing?

Okay, I see the intention behind it... how about this:

@# Lint files that have been changed only @# TODO: Update all existing files to conform to the schema

If I use the word 'TODO' then @okurz will reject the PR ;-)

Also the message "Update all existing files to conform to the schema" is wrong. They conform to the schema but they don't conform to the yamllint rules.

I would like to repeat that it was like that before, and I didn't change that.

Leave it as is. It just results in slight confusion.

rwx788 · 2020-05-04T07:17:34Z

Let's merge or is anything we still should consider doing before that?

foursixnine · 2020-05-04T08:09:56Z

Let's merge or is anything we still should consider doing before that?

I think we're good to go

perlpunk requested review from kalikiana and rwx788 April 30, 2020 10:45

perlpunk mentioned this pull request Apr 30, 2020

Replace YAML::Tiny with YAML::PP #10157

Merged

perlpunk force-pushed the json-schema branch 2 times, most recently from 4917ca9 to 28d66b2 Compare April 30, 2020 11:13

rwx788 reviewed Apr 30, 2020

View reviewed changes

foursixnine reviewed Apr 30, 2020

View reviewed changes

perlpunk force-pushed the json-schema branch 3 times, most recently from def633c to 2667518 Compare April 30, 2020 12:51

perlpunk marked this pull request as ready for review April 30, 2020 12:51

perlpunk force-pushed the json-schema branch from 2667518 to 8b83f96 Compare April 30, 2020 13:27

foursixnine approved these changes Apr 30, 2020

View reviewed changes

kalikiana reviewed Apr 30, 2020

View reviewed changes

kalikiana approved these changes Apr 30, 2020

View reviewed changes

rwx788 merged commit bd6af6e into os-autoinst:master May 4, 2020

perlpunk deleted the json-schema branch May 26, 2020 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate YAML schedules against JSON Schema #10160

Validate YAML schedules against JSON Schema #10160

perlpunk commented Apr 30, 2020 •

edited

rwx788 Apr 30, 2020

perlpunk Apr 30, 2020

rwx788 left a comment

foursixnine Apr 30, 2020

perlpunk Apr 30, 2020

rwx788 Apr 30, 2020

perlpunk Apr 30, 2020

perlpunk Apr 30, 2020 •

edited

foursixnine Apr 30, 2020 •

edited

rwx788 Apr 30, 2020

foursixnine Apr 30, 2020

perlpunk commented Apr 30, 2020

rwx788 commented Apr 30, 2020

foursixnine commented Apr 30, 2020

kalikiana Apr 30, 2020

kalikiana Apr 30, 2020

perlpunk Apr 30, 2020

kalikiana Apr 30, 2020

perlpunk Apr 30, 2020

kalikiana Apr 30, 2020

rwx788 commented May 4, 2020

foursixnine commented May 4, 2020

Validate YAML schedules against JSON Schema #10160

Validate YAML schedules against JSON Schema #10160

Conversation

perlpunk commented Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwx788 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk Apr 30, 2020 • edited

Choose a reason for hiding this comment

foursixnine Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk commented Apr 30, 2020

rwx788 commented Apr 30, 2020

foursixnine commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwx788 commented May 4, 2020

foursixnine commented May 4, 2020

perlpunk commented Apr 30, 2020 •

edited

perlpunk Apr 30, 2020 •

edited

foursixnine Apr 30, 2020 •

edited