#16 New message type for the pull request artifact
Merged 5 years ago by psss. Opened 5 years ago by psss.

@@ -0,0 +1,32 @@ 

+ {

+     "ci": {

+         "name": "Zdravomil",

+         "team": "Userspace Containerization",

+         "url": "https://somewhere.com",

+         "docs": "https://somewhere.com/user-documentation",

+         "irc": "#containerization",

+         "email": "containerization@somewhere.com",

+         "version": "1.2"

+     },

+     "run": {

+         "url": "https://somewhere.com/job/1234",

+         "log": "https://somewhere.com/job/1234/log"

+     },

+     "artifact": {

+         "type": "fedora-dist-git",

+         "repository": "https://src.fedoraproject.org/rpms/mariadb/",

+         "id": 2,

+         "comment_id": 0,

+         "commit_hash": "01a299ff20b3af5702c8c588f15eaa13945c44ca",

+         "issuer": "betka",

+         "uid": "8319283712831928731923182"

+     },

+     "type": "dockerfile-linter",

+     "category": "static-analysis",

+     "status": "failed",

+     "recipients": [ "user-cont", "dhodovsk" ],

+     "namespace": "user-cont.zdravomil",

+     "xunit": "https://somewhere.com/job/1234/artifacts/results.xml",

+     "generated_at": "2018-05-10 08:58:31.222602",

+     "version": "0.1.0"

+ }

@@ -0,0 +1,31 @@ 

+ {

+     "ci": {

+         "name": "Zdravomil",

+         "team": "Userspace Containerization",

+         "url": "https://somewhere.com",

+         "docs": "https://somewhere.com/user-documentation",

+         "irc": "#containerization",

+         "email": "containerization@somewhere.com",

+         "version": "1.2"

+     },

+     "run": {

+         "url": "https://somewhere.com/job/1234",

+         "log": "https://somewhere.com/job/1234/log"

+     },

+     "artifact": {

+         "type": "fedora-dist-git",

+         "repository": "https://src.fedoraproject.org/rpms/mariadb/",

+         "id": 2,

+         "comment_id": 0,

+         "commit_hash": "01a299ff20b3af5702c8c588f15eaa13945c44ca",

+         "issuer": "betka",

+         "uid": "8319283712831928731923182"

+     },

+     "type": "dockerfile-linter",

+     "category": "static-analysis",

+     "reason": "Internal Zdravomil error",

+     "recipients": [ "user-cont", "dhodovsk" ],

+     "namespace": "user-cont.zdravomil",

+     "generated_at": "2018-05-10 08:58:31.222602",

+     "version": "0.1.0"

+ }

file modified
+6
@@ -63,6 +63,12 @@ 

              - production

              - stage

          type: string

+     version:

+         description:

+             Version of the CI system.

+         examples:

+             - 1.2

+         type: string

  

  required:

      - name

@@ -0,0 +1,50 @@ 

+ $id: https://pagure.io/fedora-ci/messages/pull-request.test.complete

+ $schema: http://json-schema.org/draft-07/schema#

+ 

+ description:

+     Testing of the pull request has been completed.

+     This is a mandatory message.

+ 

+ properties:

+     ci:

+         $ref: contact.json

+     run:

+         $ref: run.json

+     artifact:

+         $ref: pull-request.json

+     type:

+         $ref: common.json#properties/type

+     category:

+         $ref: common.json#properties/category

+     status:

+         $ref: common.json#properties/status

+     label:

+         $ref: common.json#properties/label

+     web_url:

+         $ref: common.json#properties/web_url

+     xunit:

+         $ref: common.json#properties/xunit

+     recipients:

+         $ref: common.json#properties/recipients

+     note:

+         $ref: common.json#properties/note

+     namespace:

+         $ref: common.json#properties/namespace

+     generated_at:

+         $ref: common.json#properties/generated_at

+     version:

+         $ref: common.json#properties/version

+ 

+ required:

+     - ci

+     - run

+     - artifact

+     - type

+     - category

+     - status

+     - xunit

+     - namespace

+     - generated_at

+     - version

+ 

+ type: object

@@ -0,0 +1,49 @@ 

+ $id: https://pagure.io/fedora-ci/messages/pull-request.test.error

+ $schema: http://json-schema.org/draft-07/schema#

+ 

+ description:

+     Testing of the pull request has aborted, it was not finished, e.g.

+     because of an infrastructure error or CI system error. Note that a

+     test failure is not an error. Test failures should be exposed under

+     the pull-request.test.error topic. This is a mandatory message.

+ 

+ properties:

+     ci:

+         $ref: contact.json

+     run:

+         $ref: run.json

+     artifact:

+         $ref: pull-request.json

+     type:

+         $ref: common.json#properties/type

+     category:

+         $ref: common.json#properties/category

+     label:

+         $ref: common.json#properties/label

+     reason:

+         $ref: common.json#properties/reason

+     recipients:

+         $ref: common.json#properties/recipients

+     issue_url:

+         $ref: common.json#properties/issue_url

+     note:

+         $ref: common.json#properties/note

+     namespace:

+         $ref: common.json#properties/namespace

+     generated_at:

+         $ref: common.json#properties/generated_at

+     version:

+         $ref: common.json#properties/version

+ 

+ required:

+     - ci

+     - run

+     - artifact

+     - type

+     - category

+     - reason

+     - namespace

+     - generated_at

+     - version

+ 

+ type: object

@@ -0,0 +1,60 @@ 

+ $id: https://pagure.io/fedora-ci/messages/pull-request

+ $schema: http://json-schema.org/draft-07/schema#

+ 

+ description:

+     Details about the pull request being tested.

+ 

+ properties:

+     type:

+         description:

+             Artifact type, in this case 'fedora-dist-git'.

+         enum:

+             - fedora-dist-git

+         type: string

+     id:

+         description:

+             Human readable pull request ID.

+         examples:

+             - 123

+         type: integer

+     uid:

+         description:

+             Pull request id unique across the whole Pagure instance.

+         examples:

+             - "8319283712831928731923182"

+         type: string

+     comment_id:

+         description:

+             Comment number in the pull request.

+         examples:

+             - 1

+         type: integer

+     commit_hash:

+         description:

+             Hash of the newest commit.

+         examples:

+             - 01a299ff20b3af5702c8c588f15eaa13945c44ca

+         type: string

+     repository:

+         description:

+             Url of the tested repository.

+         examples:

+             - https://src.fedoraproject.org/rpms/mariadb/

+         type: string

+         format: uri

+     issuer:

+         description:

+             Pull request issuer.

+         examples:

+             - betka

+         type: string

+ 

+ required:

+     - type

+     - id

+     - comment_id

+     - commit_hash

+     - repository

+     - issuer

+ 

+ type: object

For now adding just 'complete' and 'error'.
Optional 'version' added to the 'contact' schema.

Regarding naming: Instead of dist-git-pr I suggest to use more general pull-request as the name for the messages. This would give us flexibility to use this message type for other Pagure instances as well. The artifact type could be used to identify the instance and/or repository attribute could hold the whole url to make the repo uniqe. What do you think?

I also added an optional field version to the contact schema as the dist-git-pr example has it there. Is that ok? Finally, the uid attribute seems to be a number. Shall we update the spec to require integer there?

@mvadkert, @dhodovsk, please review.

I am not against using pull-request name for the topic name, if everyone agrees. After the change we need to update it in it in all known listeners of the message . For now it is pagure for resulting flags and kibana for metrics. I am not sure about any other active listeners of this topic.

The version tag in CI should be mandatory according to (docs)[https://docs.google.com/document/d/16L5odC-B4L6iwb9dp8Ry0Xk5Sc49h9KvTHrG86fdfQM/edit#heading=h.s4lk82dr6dl0]

I asked Slavek about the uid and we should expect string, since it is generated using uuid (or sth similar)

Regarding the version field: I see. I tried to reuse the contact schema which is currently used in basically all other messages. If we need to introduce a mandatory version field we will have to fork this schema or make version mandatory for all other as well.

Just to make sure there is not a confusion: We have already a mandatory version field included in the message body:

For the dist-git-pr messages you want to have a separate version field under the ci, right?

Oh, I forgot about it. But I also can't find in my memory the reasoning behind having version in CI, when it is already in the body of the message.

Since it would be more complex now, let's make it optional since it is not used by Zdravo anyway.

I now found the comment in docs about the type of ID (not uid used internaly by pagure) and human readable PR id can indeed be integer indeed.

Oh, I forgot about it. But I also can't find in my memory the reasoning behind having version in CI, when it is already in the body of the message.

The version field in the message body specifies version of the message format. So that does not relate to the CI system.

Since it would be more complex now, let's make it optional since it is not used by Zdravo anyway.

OK, good. If the usage is not clear would it make sense to remove it from messages?

I now found the comment in docs about the type of ID (not uid used internaly by pagure) and human readable PR id can indeed be integer indeed.

Fine, then we can keep the current definition of id. Thanks for the review. Do you have any other questions/comments?

Do you think we can support the PR update event here?

When I am updating pull request, for example with rebase and force push, the PR id and pr are going to be the same, but it is a different event.

And on the consumer side, for example for CI systems, the updated pull request should trigger the cancelling logic: whatever is running for the previous version of the pull request should be cancelled.

To implement it we need certain ordering of artifacts. For example additional counter(aka version) in the artifact schema which is going to be increased with every PR update. Otherwise we can not decide which version comes later.

 "artifact": {
         "type": "dist-git-pr",
         "comment_id": 0,
         "commit_hash": "01a299ff20b3af5702c8c588f15eaa13945c44ca",
         "issuer": "betka",
         "repository": "rpms/rh-mariadb102-docker",
         "uid": "8319283712831928731923182",
         "id": 2,
         "version": 1
}

I think pull request update could be handled without additional version field. If we use commit hash as a unique identifier we could present that information in the pull request side bar as it is done by the simple koji:

Also disabling any previously running jobs for the same pull request should be possible without knowing the exact order of the commits. Or, possibly, the generated_at field could be used to do the sorting if needed.

@psss even in that example link you show, commit sha is not really helpful. It does show that test runs are different, but it doesn't ensure that the order of the results is right.

generated_at field can be used to sort messages, but I can imagine some weird things happening on the infrastructure, which cause messages to be delayed or lost or reordered which makes this unreliable source of information.

We probably need to ask @pingou if there is a concept of a version for a pull request in Pagure. If not, then I agree inventing in just in the message is not going to help.

Then we need to add it as a feature to the Pagure first.

@psss even in that example link you show, commit sha is not really helpful. It does show that test runs are different, but it doesn't ensure that the order of the results is right.

From what I can see, currently the flags are listed in the correct order (as they come). So you always find the latest at the bottom. Plus the commit hash helps when looking for a specific rebase/commit if there are multiple results. So I'm not sure what's the issue we're trying to solve.

From what I can see, currently the flags are listed in the correct order (as they come).

1) It is not the correct order :)

As a developer I am interested in my current state. Only results of the latest version of my PR are important. Everything else is just historical data, which can be used for debugging, but generally is just a noise. Ideally it should be collapsed and available only on request.

2) Imagine the race condition: there was a PR, it triggered the test. Now i realized that I broke the test in this PR and created new PR version, without waiting for test to finish. It triggered new test and got results much faster (because test was fixed). While test for my previous version of pr was still in progress. Then finally the test for previous PR version was killed by timeout and we got the result for the older version of PR.

How results are going to look?

3) The generated_at field belongs to the message schema, not the artifact. Which means given by two artifact jsons alone, separate from the messages wrapping them, we can not find which artifact is the latest. Which means the artifact schema is not descriptive enough.

Hmm, I thought comment id get's incremented for each pull request. even as a rebase?

cc @bkabrda

Yes, each comment has an id, including rebase comments, see:
https://src.fedoraproject.org/api/0/rpms/python3/pull-request/45
So I believe comment_id could be used to track the correct order.

@psss @mvadkert Ok, thanks. It looks a bit counter intuitive to me to be honest, but close enough.

We are also looking forward to use the proposed pull-request artifact type. Our use case is described in https://pagure.io/greenwave/issue/332.

I am currently not sure if the spec proposed in this PR fits all we need. I will take a closer look soon.

@rayson, I've reviewed the use case linked above and I think it nicely aligns (in point 3) with the proposed pull-request message. Feel free to comment and suggest any adjustments which would be needed to make this smoothly working with greenwave.

Can we have a general case for non-dist git PR? Perhaps just git-pr?

Yes, I think the type attribute is the right place to specify which type of git repository is concerned. Would it make sense to have a different type for each pagure (github, gitlab) instance? Or to use dist-git-pr for both upstream and downstream dist git? Thoughts?

As discussed with @psss in person, I think to have separate types for different instances makes more sense for now. Currently, dist-git-pr is used for downstream dist-git. For Fedora's dist-git we should come up with a new type. Same applies to any additional git instances.

@psss Looks good. Do you think if custom data field is needed?

If this artifact represents a generic pull-request, not all pull-requests have a uid, right?

Can this field be optional?

Looks good. Do you think we should add more properties, like:
- web_url: a URL to the web interface of the pull-request
- pseudo_branch: a pseudo branch like pull/<PR No>/head for fetching the code of this pull-request.
- author: The submitter/contact of the pull-request.
- created_at: The date time when this pull-request is created.

rebased onto 596c7bb7e61b442bdbeadf50a8782b293bc4a7ab

5 years ago

rebased onto 928f8de20a7eba78bd430f551522aa11a38ce9dc

5 years ago

I was thinking a bit more about handling the different instances and here's my proposal: Could we try as the first step introducing for Fedora new message topic pull-request.test.* with the artifact type set to fedora-dist-git? This is different from what we have downstream (so no collision there) but could be used to prove if this approach is viable. I've updated the pull request to match this proposal.

@rayson, regarding the fields: I'm not sure the uid field could be made optional. It seems we should be able to identify the pull request by repository and id as well. @dhodovsk might know more details.

For the comment_id, I think this one is definitely necessary as we want to know which comment triggered the testing. The other attributes you are suggesting could be added as optional fields (if there is a clear use case for them).

Hi @psss,

My concerns are about using other code review services other than Pagure (like GitHub, Gitlab), those fields may not available.

rebased onto 643457b

5 years ago

Thanks for clarifying, @rayson. As suggested in some of the earlier comments, I think if we use the whole url to identify repository plus unique pull request id then we could make the uid optional and could be used for Pagure instances only. I've updated the pull request accordingly.

@dhodovsk, @mvadkert, what do you think?

@psss

rayson commented a day ago

Hi @psss,

My concerns are about using other code review services other than Pagure (like GitHub, Gitlab), those fields may not available.


Yeah, this is exactly why it makes sense to have it as separate artifcats for different git forges. I think having pagure-pull-request, gitlab-pull-request, github-pull-request would be better. If we go with too generic artifact types, we will get into not clear specifciation (we will need to make some field required if the git is this with this type ..)

@psss but I let decision to you, I do not want to block this more

Metadata Update from @psss:
- Pull-request tagged with: pull-request
- Request assigned

5 years ago

OK, let's try it this way. We'll see how it works.

Pull-Request has been merged by psss

5 years ago