@mvadkert Kindly review and let me know if any changes required.

Where is the documentation for the syntax?

The #default comments are unnecessary.

@lholecek
Syntex can be found here,
camel simple Language: https://camel.apache.org/components/latest/languages/simple-language.html

Regarding the Default comment, sure will address them.

Syntex can be found here,
camel simple Language: https://camel.apache.org/components/latest/languages/simple-language.html

Can you add the link to README file?

@lholecek Sure thing.. will add that.

Syntex can be found here,
camel simple Language: https://camel.apache.org/components/latest/languages/simple-language.html

I only quickly skimmed the document but I cannot find any mention about the #if(...)...#{else}...#end statement. What am I missing?

@lholecek
We are using Simple Language for substitutions, transformations, etc. The link which @draval mentioned talks about it.
For conditionals, we are using the Velocity Template Engine. More information could be found here http://people.apache.org/~henning/velocity/html/index.html
Check the 5.3 section about conditionals.

If we need more flexibility w.r.t. using velocity, we can make a JSON file instead of YAML.
We are adding the links in readme.

run.url has nothing to do with testcase, it is basically a reference to the CI run, as schema says Specific build URL (e.g. Jenkins build URL).

hmm, I do no think this is a good idea, I would just use here ${body.run.url}

I wonder, why we are not following the standard here and need to map the fields in such a weird way? For example this field would beartifact_nvr

@draval thinking about this more, is there any reason why we, for the fields which are not specific to ResultsDB just use the mapping similarly to the standard?

So for example this:

18   artifact: {
 19     type: brew-build,
 20     id: 14546276,
 21     issuer: ovasik,
 22     component: setup,
 23     nvr: setup-2.8.71-7.el7_4,
 24     scratch: true,
 25     dependencies: [gcc-3.8.2-1.el7, make-2.3.1-2.el7_4],
 26     baseline: setup-2.8.71-6.el7_4,
 27     source: git+https://src.fedoraproject.org/rpms/setup.git?#5e0ae23a
 28   },

Would map to

artifact.type: "brew-build"
artifact.id: "14546276"
artifact.issuer: "ovasik"
....

Also noting that @adamwill was having a defined way how to store these stuff in resultsdb, so maybe he could chime in here ... we are afterall mapping messages to resultsdb results, thus I believe that was exactly about it ...

FTR, this is what I was talking about in the above comment: https://pagure.io/taskotron/resultsdb_conventions/

run.url has nothing to do with testcase, it is basically a reference to the CI run, as schema says Specific build URL (e.g. Jenkins build URL).

what you would like to configure as ref_url then?

@draval thinking about this more, is there any reason why we, for the fields which are not specific to ResultsDB just use the mapping similarly to the standard?

So for example this:

18 artifact: { 19 type: brew-build, 20 id: 14546276, 21 issuer: ovasik, 22 component: setup, 23 nvr: setup-2.8.71-7.el7_4, 24 scratch: true, 25 dependencies: [gcc-3.8.2-1.el7, make-2.3.1-2.el7_4], 26 baseline: setup-2.8.71-6.el7_4, 27 source: git+https://src.fedoraproject.org/rpms/setup.git?#5e0ae23a 28 },

Would map to

artifact.type: "brew-build" artifact.id: "14546276" artifact.issuer: "ovasik" ....

@mvadkert
Though I agree with more logical naming convection of the fields.
The reason behind this,We tried to keep the mapper field name same as ResultsDB V1(Existing).
As when someone want to create a mapping file they simply going to use the existing mapping as reference and If we change the field name completely it may create some confusion or understanding gap while creating the mapping fields.

Existing Mapping you can see here: https://github.com/release-engineering/resultsdb-updater/blob/8951154c057beb5a080da2bf2c2448cef69446aa/resultsdbupdater/utils.py#L491

@taziz feel free to add if you think otherwise

I believe almost nobody uses the current data field from resultsdb internally, do we know about somebody using that information anyhow?

greenwave does use resultsdb data. which fields depends on configuration...

@adamwill yeah, I know that, but I thought it is only:

• item
• testcase
• outcome

Does it rely on anything from the data field?
https://resultsdb20.docs.apiary.io/#reference/0/results

Does it rely on anything from the data field?

Greenwave usually depends on item and type from the data field stored in ResultsDB.

Additionally, there are following data fields which help to identify unique results:
- scenario
- system_architecture
- system_variant

And also any special fields mentioned in subject type configuration (result_queries and item_key config keys). E.g. original_spec_nvr for koji_build or productmd.compose.id for compose.

@mvadkert
Also naming convection under the "data" object is loosely coupled and one can use as per their convenience, so if you would like to use artifact_nvr instead item then it can be specific to your mapping file.

However, it will create an inconsistency with regards to other mapping files.. though there is no such impact on the functionality.

So kindly confirm the field name which you would like to change and I can update them.

@lholecek thanks for the info :) how realistic would be greenwave to support changes to these? so we can move to something more reasonable?

@mvadkert
Also naming convection under the "data" object is loosely coupled and one can use as per their convenience, so if you would like to use artifact_nvr instead item then it can be specific to your mapping file.

while I understand that, still I find valuable to standardize to some reasonable scheme

However, it will create an inconsistency with regards to other mapping files.. though there is no such impact on the functionality.

right, I would like to avoid inconsistencies as much as possible

So kindly confirm the field name which you would like to change and I can update them.

I would like us to move to a more reasonable scheme that we have now, let's wait for @lholecek if that is possible, or we just cannot change the current scheme due to Greenwave

how realistic would be greenwave to support changes to these?

I think the result data would also need to be versioned and Greenwave would need to support multiple versions. This would complicate and possibly slow down the ResultsDB queries that Greenwave needs to make.

Alternatively, there would be a new subject/item type and new configuration in Greenwave but this would require change in clients.

yeah, the tricky thing with how it works is that greenwave clients can ask greenwave for more or less anything, so it's not really so much a case of "is greenwave OK with this?" but "are all the greenwave clients we care about OK with this?" Also clients can pass the 'verbose' option when asking greenwave for a decision, which causes greenwave to actually pass along the entire set of relevant resultsdb results, and the client can then parse those however it likes.

The one I'm familiar with of course is Bodhi in Fedora, and for instance you can see Bodhi getting verbose decisions and then doing stuff with the result data - in that case, the 'scenario'. There may well be other cases like that.

@lholecek
Addressed the review comments and Updated the Readme file with syntax.

@mvadkert
As discussed, Updated Mapping file for brew-build.test.complete.yaml with a minimal template.
Also added brew-build.test.error, brew-build.test.queued & brew-build.test.running yamlfiles as well.

+1

I believe outcome should be error or ERROR

https://pagure.io/fedora-ci/messages/blob/master/f/schemas/test-complete.yaml#_8

test.status.result is usable only for completed runs (failed / passed / info, ...)

same here, outcome should be QUEUED or queued (not sure if it needs to be uppercase

and this needs to be RUNNING or running

Mapping file attributes, does not need to start with capital A

Also here logic would be prefered

@mvadkert
Regarding test.error,queued & running.
As you suggested changes for the outcome, do you see any changes for ref_url & item as well?
Because all these fields are referring specific attributes in messages.

so in case of error/queued/running, if run orartifact object is missing we will end up with garbage text in results db.

sample message would be really helpful for error,queueud& running.

@tflink is there a range of expected values for outcome that's documented anywhere?

The built-in outcomes are:
- PASSED
- INFO
- FAILED
- NEEDS_INSPECTION

This can be extended with the ADDITIONAL_RESULT_OUTCOMES option.

All outcomes are listed using API: https://taskotron.fedoraproject.org/resultsdb_api/api/v2.0/

Here is how Greenwave uses the outcome value: https://docs.pagure.org/greenwave/decision_requirements.html

We definitely need to add at least: RUNNING, QUEUED and ERROR to that mix to be able to comply with this standard, and this is also what we do in RH. In Fedora CI we are in process of getting in those states also, to have the best experience for the users.

there is one annoyance here:

https://pagure.io/fedora-ci/messages/blob/master/f/schemas/test-complete.yaml#_19

can we map this to info? not_applicable is not something resultsdb recognizes :disappointed:

@mvadkert

Please Review. I have addressed the changes requested.

does this needs to be upper-case? The ${body.test.result} is lower, case, maybe if it does not matter, let's use lower-case for consistency?

@mvadkert
Regarding test.error,queued & running.
As you suggested changes for the outcome, do you see any changes for ref_url & item as well?
Because all these fields are referring specific attributes in messages.

so in case of error/queued/running, if run orartifact object is missing we will end up with garbage text in results db.

sample message would be really helpful for error,queueud& running.

I believe no input from me needed here anymore right?

@mvadkert
Yes, uppercase will be consistent with ${body.test.result}. As we always send in Uppercase to resultsdb after transformation.

@mvadkert
Regarding test.error,queued & running.
As you suggested changes for the outcome, do you see any changes for ref_url & item as well?
Because all these fields are referring specific attributes in messages.

so in case of error/queued/running, if run orartifact object is missing we will end up with garbage text in results db.

sample message would be really helpful for error,queueud& running.

I believe no input from me needed here anymore right?

I guess no need now..!!

does this needs to be upper-case? The ${body.test.result} is lower, case, maybe if it does not matter, let's use lower-case for consistency?

No need to have it upper-case here; ResultsDB converts it to upper-case automatically (basically calls outcome.strip().upper()).

not_applicable is not something resultsdb recognizes

@mvadkert Internally, ResultsDB recognizes NOT_APPLICABLE but Greenwave parses it as a failed test. I guess this test result is not used in Fedora.

Can you open a new issue for Greenwave to discuss this? Handling specific test results are currently hard-coded in Greenwave, it would be nice to have it in configuration anyway.

+1

@lholecek on it (not_applicable)

So seems we are all set here afaict.

so :thumbsup: from me also

Do we need anybody else to sign-off

@ralph @msrb @adamw @bgoncalv anybody else want to sign off?

I believe we are clean now

I'm not really 100% sure but I think I'm fine with you going ahead, it'll be easier to see any issues once it's deployed...

:+1: I'm late to the party, but it looks like there's agreement here. Merging!

@ralph / @mvadkert
We also need new tag 1.x.x for this PR.

@draval I will tag it to 1.1.0

@draval it is tagged: https://pagure.io/fedora-ci/messages/releases

:birthday: