Enhance verify method in recipe.py

[junaruga]

I enhanced the verify method. That must assist users to create a recipe file.

And one more point. COLLECTION_ID/name such as "name: Ruby on Rails 5.0" was a required element last time. But regarding below document, it is an optional element. https://github.com/sclorg/rhscl-rebuild-recipes So, I changed the element from "required" to "optional".

[junaruga]

@khardix Thanks for mentioning it. I fixed the issues, and rebased on latest master branch.

[khardix]

@junaruga Could you do something about the point 1 from my previous comment?

1. Since it raises on each failure, the user learns his errors one at a time, which is incredibly tedious. I strongly suggest collecting the errors during the function and report them all at once at the end.

I you do not have time or an idea what to do with it, tell me and I will take a shot at it.

[junaruga]

Since it raises on each failure, the user learns his errors one at a time, which is incredibly tedious. I strongly suggest collecting the errors during the function and report them all at once at the end.

Sorry I missed your comment. Sure. That makes sense. Let me modify for that. Thanks.

[junaruga]

The whole function is basically just bunch of ad-hoc ifs and starts to become pretty unreadable. I would consider using a dedicated data validation library for this. I have a good experience with cerberus.

Yeah, bunch of ad-hoc if is not readable. I wanted to use kind of switch statement if Python had it.

As unfortunately I am not familiar with cerberus, I want to ask you to fix with cerberus after this PR.

[junaruga]

As unfortunately I am not familiar with cerberus, I want to ask you to fix with cerberus after this PR.

I try to do this with cerberus by mysef :)

[junaruga]

I try to do this with cerberus by mysef :)

I tried, but sadly cerberus does not support union value (a list of union (string or dict), union (string or list), and etc.) We can watch that will fixed. https://github.com/pyeve/cerberus/pull/321

As we are using the union value for packages and cmd, it's hard to implement with cerberus right now. Let's implement manually.

[junaruga]

Ah, no maybe I can implement with some hacks.

[khardix]

I tried, but sadly cerberus does not support union value (a list of union (string or dict), union (string or list), and etc.)

If I understood you correctly, this should be possible by using something like

schema = {'type': 'list', 'schema': {'anyof_type': ('string', 'dict')}}

Edit: After looking at the linked PR and issues, I see the bug involved. I would try replacing 'oneof' with 'anyof' and see what that does. If that does not help, lets just replace the exceptions (point 1) and leave the refactoring (point 2) an open issue waiting for next release of cerberus.

[junaruga]

Ah I have used 'oneof', not 'anyof'. But 'anyof' is correct. I will try anyof. Thanks for the information.

Right now I pushed additional commit in work, to share my current status with you. though I still need more work for the implementation. This modification needs 2 schema data and the cerberus validation check 2 times for above reported bug.

Here is the branch I tried with 1 schema data as a reference. https://github.com/junaruga/rpm-list-builder/blob/feature/enhance-recipe-verify-work/rpmlb/recipe.py

[junaruga]

I would try replacing 'oneof' with 'anyof' and see what that does.

Thanks. After the trying, let me know. :)

If that does not help, lets just replace the exceptions (point 1) and leave the refactoring (point 2) an open issue waiting for next release of cerberus.

Okay, I think at least we can use cerberus practically like my latest commit.

[junaruga]

@khardix I want your advice for below situation.

I modified ror.yml file to check the error messages.

$ git diff
diff --git a/ror.yml b/ror.yml
index 4eba927..3c73dcf 100644
--- a/ror.yml
+++ b/ror.yml
@@ -23,11 +23,15 @@ rh-ror50:
     # Packages required by RSpec, Cucumber
     - rubygem-rspec
     - rubygem-rspec-core:
+        macros: MACROS
         replaced_macros:
           need_bootstrap_set: 1
+        foo: FOO
     - rubygem-rspec-support:
         replaced_macros:
           need_bootstrap_set: 1
+        bar: BAR
+        patch: PATCH
     - rubygem-diff-lcs:
         macros:
           # Temporary update because of the RPM spec file issue.

(venv) $ rpmlb \
  --download rhpkg \
  --build dummy \
  --mock-config rhscl-2.4-rh-ror50-rhel-7-x86-64 \
  --branch rhscl-2.4-rh-ror50-rhel-7 \
  --verbose \
  ../rhscl-rebuild-recipes/ror.yml \
  rh-ror50 2>&1 | tee rpmlb.log
...
  File "/home/jaruga/git/rpm-build-tool/rpm-list-builder2/rpmlb/recipe.py", line 160, in verify
    raise RecipeError(validator.errors)
rpmlb.recipe.RecipeError: {'packages': [{2: [{'foo': ['unknown field'], 'macros': ['must be of dict type']}], 3: [{'bar': ['unknown field']}]}]}

Error message is like this. 2, 3 is the element order number of the list. I want to output rubygem-rspec-core, rubygem-rspec-support too. Also I want to output warning: deprecated element for patch element. In this case, I want to implement it manually from errors object? is there any convenient way in cerberus for that?

packages rubygem-rspec-core [num:2]: foo: unknown field.
packages rubygem-rspec-core [num:2]: macros: must be of dict type.
packages rubygem-rspec-support [num: 3]: bar: unknown field.
packages rubygem-rspec-support [num: 3]: patch: deprecated element. Use cmd element instead.

[junaruga]

If that does not help, lets just replace the exceptions (point 1)

I will just replace current exception logic to show multi error messages without cerberus. Right now cerberus does not customize the message. But maybe we need more user friendly error message.

and leave the refactoring (point 2) an open issue waiting for next release of cerberus.

I will leave the refactoring here on this branch of mine. https://github.com/junaruga/rpm-list-builder/blob/feature/enhance-recipe-verify-work2/rpmlb/recipe.py

open issue waiting for next release of cerberus.

Let's watch below 2 pages.

https://github.com/pyeve/cerberus/pull/321
https://github.com/pyeve/cerberus/issues/329

[junaruga]

After fixing those, and finishing my implementation, let you know.

[junaruga]

@khardix I fixed 2 points that you mentioned.

Review please.

(venv) $ rpmlb \
  --download rhpkg \
  --build dummy \
  --mock-config rhscl-2.4-rh-ror50-rhel-7-x86-64 \
  --branch rhscl-2.4-rh-ror50-rhel-7 \
  ../rhscl-rebuild-recipes/ror.yml \
  rh-ror50
Error: Recipe file is invalid.
packages[3] rubygem-rspec-core macros should be a mappings.
packages[3] rubygem-rspec-core foo is an unknown element.
packages[4] rubygem-rspec-support bar is an unknown element.
packages[4] rubygem-rspec-support patch is an unknown element.
(venv) $ echo $?
1

[khardix]

Good work! Approved for merge with a squash.

[junaruga]

Thanks for the review. I merged with the squash.