PR#123: Add missing fields to all brew-build messages - fedora-ci/messages

fedora-ci / messages

#123 Add missing fields to all brew-build messages

Merged 2 years ago by lholecek. Opened 2 years ago by pholica.

fedora-ci/ pholica/messages master into master

Add missing fields to rest brew-build results

Pavel Holica • 2 years ago

002e895

mappings/results/brew-build.test.error.yaml

file modified

+10

		`@@ -9,3 +9,13 @@`
		`- "brew-build"`
		`item:`
		`- "${body.artifact.nvr}"`
		`+ system_provider:`
		`+ - "${body.system[0].provider}"`
		`+ system_architecture:`
		`+ - "${body.system[0].architecture}"`
		`+ system_variant:`
		`+ - "${body.system[0].variant}"`
		`+ scenario:`
		`+ - "#if(${body.test.scenario})${body.test.scenario}#{else}not available#end"`
		`+ scratch:`
		`+ - "${body.artifact.scratch}"`

mappings/results/brew-build.test.queued.yaml

file modified

+10

		`@@ -9,3 +9,13 @@`
		`- "brew-build"`
		`item:`
		`- "${body.artifact.nvr}"`
		`+ system_provider:`
		`+ - "${body.system[0].provider}"`
		`+ system_architecture:`
		`+ - "${body.system[0].architecture}"`
		`+ system_variant:`
		`+ - "${body.system[0].variant}"`
		`+ scenario:`
		`+ - "#if(${body.test.scenario})${body.test.scenario}#{else}not available#end"`
		`+ scratch:`
		`+ - "${body.artifact.scratch}"`

mappings/results/brew-build.test.running.yaml

file modified

+10

		`@@ -9,3 +9,13 @@`
		`- "brew-build"`
		`item:`
		`- "${body.artifact.nvr}"`
		`+ system_provider:`
		`+ - "${body.system[0].provider}"`
		`+ system_architecture:`
		`+ - "${body.system[0].architecture}"`
		`+ system_variant:`
		`+ - "${body.system[0].variant}"`
		`+ scenario:`
		`+ - "#if(${body.test.scenario})${body.test.scenario}#{else}not available#end"`
		`+ scratch:`
		`+ - "${body.artifact.scratch}"`

pholica commented 2 years ago

Some parts that were lost in previous communication as part of https://pagure.io/fedora-ci/messages/pull-request/121

pholica commented 2 years ago

@draval @lholecek
Please have a look. I hope this will finally make the brew-build working in GreenWave.

draval commented 2 years ago

@pholica @lholecek
Looks good to me.

lholecek commented on line 6 of mappings/results/brew-build.test.complete.yaml 2 years ago

@draval, @pholica Is this guaranteed to be "brew-build" in the message? I do not think it is required by schemas.

Should this be set to "brew-build"? Like in #122 - which I will drop later.

pholica commented 2 years ago

@lholecek Good question. Sorry I missed the #122, I had this previously in #120 :D
Well, according to https://pagure.io/fedora-ci/messages/blob/master/f/schemas/rpm-build.yaml the type is mandatory and should be either "brew-build" or "koji-build" as it's used in both messages I presume. I'm not sure what's the desired level of consistency, but I don't have a problem hardcoding the "brew-build" there as it's already in the message topic. I don't have any real preference for this.

pholica commented 2 years ago

Having that said, please let me know if you prefer it hardcoded or taken from the message. I believe there's really no good solution if the message mismatches topic and artifact type, but if it's the same I don't see a difference in the importer code (other than the brew-build could be possibly just symlinked to koji-build if it's not hardcoded).

draval commented 2 years ago

@lholecek @pholica
If I understood the ask here,
There is no need to hard code type=bre-build as this Mapping template will only be used for artifact = brew-build.

are we saying even for rtifact=brew-build we may get type=kojibuild?

pholica commented 2 years ago

Oh sorry, I don't see inside the importer code so I may have presumed something what's not the case. I thought that the filenames correspond with the message topics and what I meant to say, that one can still use the ci.brew-build.test.complete topic with message content saying that the artifact is "koji-build" (which is imho incorrect) but that would pass the validation of the message as it's accepted by the enum. This is the only situation which I see may have effect on the import but in that case it would make sense to not import such result as all, but that's not something this pull request should deal with.

draval commented 2 years ago

@pholica
That's right understanding.
The whole validation is Topic vs MappingFile driven.
So if topic ci.brew-build.test.complete then we will search for the "brew-build.test.complete.yaml" file and perform the validation as per the same schema & transform the message according to the mapping file.

So in given use case, where topic= ci.brew-build.test.complete and in message body artifact.type=koji-build. Then transformed message will send type=koji-build.

However, I would suggest to may be using dedicated schema & mapping for koji-build instead using of brew-build if it meets your CI requirements.

lholecek commented 2 years ago

+1 If the type is limited to "brew-build" or "koji-build", LGTM and it is good to merge.

lholecek commented 2 years ago

@mvadkert Can you also review?

I closed #122 (adds just "type" to the mappings) but you managed to merge it somehow. :)

pholica commented 2 years ago

Hmm, as #122 is merged I need to rebase this pull-request since it conflicts with it. I guess I'm going to just replace the types related commit in this pull-request with the one from #122 .

Edited 2 years ago by pholica

rebased onto 002e895

2 years ago