Tyr - pull request format checker

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

Tyr - pull request format checker

Martin Stefanko
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Jiri Pallich
Nice! 

Will raise the awareness of this on the Runtimes PM Call. I would not be afraid running it in the "private" openshift as a pilot. I agree with Darran that long-term there should be a supported way how to run and maintain this tool.

Cheers,
Jiri

On Mon, Apr 8, 2019 at 2:54 PM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


--

Jiri Pallich
Senior Program Manager
Middleware Runtimes and RHOAR (Red Hat Openshift Application Runtimes)

Tel.: +420 532 294 393
Cell: +420 775 158 118
Email: [hidden email]

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Brian Stansberry
In reply to this post by Martin Stefanko
The checks look pretty reasonable -- not overly rigid, which is good.

Does the commit check run against all commits in the PR?

On Mon, Apr 8, 2019 at 3:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


--
Brian Stansberry
Manager, Senior Principal Software Engineer
Red Hat

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Brian Stansberry
In reply to this post by Martin Stefanko
Tangent from the question about where to run it:

What happens if it is down?  I figure nothing really, i.e. the check just isn't performed, which is a quite reasonable failure mode. :) 

AIUI this also has no impact on ability to merge PRs; i.e. we can ignore the result same as we can ignore CI job results. Also good.

On Mon, Apr 8, 2019 at 7:54 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


--
Brian Stansberry
Manager, Senior Principal Software Engineer
Red Hat

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Ken Wills-2
In reply to this post by Martin Stefanko

On Mon, Apr 8, 2019 at 7:32 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

For the initial version, I think we should just try it in OS though, then we can move it at some point in the future.

Ken

 

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
Does the commit check run against all commits in the PR?

Not currently, I've added additional checks to verify there is only one commit in the PR :) But this should be possible to do, however, force pushing your way through commit history will not be an easy task.

What happens if it is down?  I figure nothing really, i.e. the check just isn't performed, which is a quite reasonable failure mode. :) 

AIUI this also has no impact on ability to merge PRs; i.e. we can ignore the result same as we can ignore CI job results. Also good.

Absolutely right. There will be only one additional status in the list. Everything can be ignored.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

Ken, I will take this with you offline when it's decided that we can set it up. 

Martin 


On Mon, Apr 8, 2019 at 4:15 PM Ken Wills <[hidden email]> wrote:

On Mon, Apr 8, 2019 at 7:32 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

For the initial version, I think we should just try it in OS though, then we can move it at some point in the future.

Ken

 

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
Overall if for a tool we don't care if it down and consider it's output to be something to ignore it would seem to offer little value, I would suggest we aim for it to be available with useful output.

Will this just be adding a comment or will this also be influencing the status of the pull request - i.e. will these reports be affecting the tick or the cross visible next to a PR?  When looking at the PR queue the ones with the green ticks are the easiest to jump into, provided no changes are required to the code they are potentially ready to merge - adding false crosses would affect this.

On Tue, Apr 9, 2019 at 6:47 AM Martin Stefanko <[hidden email]> wrote:
Does the commit check run against all commits in the PR?

Not currently, I've added additional checks to verify there is only one commit in the PR :) But this should be possible to do, however, force pushing your way through commit history will not be an easy task.

I think instead it should be checking all commits, it often makes sense to break work into logical chunks - often it is easier to review commit by commit.

As an example it is a bad practice to bump a subsystem version and add to it within a single commit, GitHub provides no way to diff files like schemas when that happens so the only way to check is to pull the PR locally and manually diff. I suspect the manual diff is a very rare event if it happens at all.  Component upgrades whilst could be in a single commit often make sense to be in individual commits.  When working on an issue and encountering a related but different bug it makes sense to use a separate Jira issue and commit, it is not always practical to submit this fix in it's own PR.  If working on a piece of work and another engineer also follows on you need to ensure you don't modify any shared commits etc... 

Overall the human code review is still mandatory and part of that review would consider the commits and if they make sense.

I think something that could be useful could be to list the checks to be applied somewhere to make it easier to discuss individually, as this would be a change to the WildFly Core and WildFly pull requests it could even make sense to submit as a proposal to the wildfly-proposals repo.
 

What happens if it is down?  I figure nothing really, i.e. the check just isn't performed, which is a quite reasonable failure mode. :) 

AIUI this also has no impact on ability to merge PRs; i.e. we can ignore the result same as we can ignore CI job results. Also good.

Absolutely right. There will be only one additional status in the list. Everything can be ignored.

Absolutely everything we add to the status list we should be aiming to be accurate, CI is complex and a never ending task to keep on top of intermittent issues and flaky tests but the objective is still to reach that point.  We should not be adding something else to this list deliberately with false negative reviews.
 

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

Ken, I will take this with you offline when it's decided that we can set it up. 

Martin 


On Mon, Apr 8, 2019 at 4:15 PM Ken Wills <[hidden email]> wrote:

On Mon, Apr 8, 2019 at 7:32 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

For the initial version, I think we should just try it in OS though, then we can move it at some point in the future.

Ken

 

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Brian Stansberry


On Tue, Apr 9, 2019 at 5:39 AM Darran Lofthouse <[hidden email]> wrote:
Overall if for a tool we don't care if it down and consider it's output to be something to ignore it would seem to offer little value, I would suggest we aim for it to be available with useful output.


Yes.
 
Will this just be adding a comment or will this also be influencing the status of the pull request - i.e. will these reports be affecting the tick or the cross visible next to a PR?  When looking at the PR queue the ones with the green ticks are the easiest to jump into, provided no changes are required to the code they are potentially ready to merge - adding false crosses would affect this.

On Tue, Apr 9, 2019 at 6:47 AM Martin Stefanko <[hidden email]> wrote:
Does the commit check run against all commits in the PR?

Not currently, I've added additional checks to verify there is only one commit in the PR :) But this should be possible to do, however, force pushing your way through commit history will not be an easy task.

I think instead it should be checking all commits, it often makes sense to break work into logical chunks - often it is easier to review commit by commit.

For sure I don't want rejection of PRs with multiple commits.  It's perfectly valid to write PRs with multiple commits.

As an example it is a bad practice to bump a subsystem version and add to it within a single commit, GitHub provides no way to diff files like schemas when that happens so the only way to check is to pull the PR locally and manually diff. I suspect the manual diff is a very rare event if it happens at all.  Component upgrades whilst could be in a single commit often make sense to be in individual commits.  When working on an issue and encountering a related but different bug it makes sense to use a separate Jira issue and commit, it is not always practical to submit this fix in it's own PR.  If working on a piece of work and another engineer also follows on you need to ensure you don't modify any shared commits etc... 

Overall the human code review is still mandatory and part of that review would consider the commits and if they make sense.

Yes. 

I see this tool as helping to encourage people to follow proper practice and save humans doing code review having to nag people over such things, or not having the energy to nag, instead accepting things they don't really want.


I think something that could be useful could be to list the checks to be applied somewhere to make it easier to discuss individually, as this would be a change to the WildFly Core and WildFly pull requests it could even make sense to submit as a proposal to the wildfly-proposals repo.
 

What happens if it is down?  I figure nothing really, i.e. the check just isn't performed, which is a quite reasonable failure mode. :) 

AIUI this also has no impact on ability to merge PRs; i.e. we can ignore the result same as we can ignore CI job results. Also good.

Absolutely right. There will be only one additional status in the list. Everything can be ignored.

Absolutely everything we add to the status list we should be aiming to be accurate, CI is complex and a never ending task to keep on top of intermittent issues and flaky tests but the objective is still to reach that point.  We should not be adding something else to this list deliberately with false negative reviews.

Yes, we want it to be accurate. We also want it to be available.

I think a key way to keep this accurate is to resist the temptation to have it do more than it should. Don't come up with rules that sound right but turn out to have valid exceptions or that people just won't follow.

OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.
 
 

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

Ken, I will take this with you offline when it's decided that we can set it up. 

Martin 


On Mon, Apr 8, 2019 at 4:15 PM Ken Wills <[hidden email]> wrote:

On Mon, Apr 8, 2019 at 7:32 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

For the initial version, I think we should just try it in OS though, then we can move it at some point in the future.

Ken

 

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


--
Brian Stansberry
Manager, Senior Principal Software Engineer
Red Hat

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
The idea is only to verify pull request structure by rules defined by Wildfly - so for instance if we require that every commit is prefixed with [WFLY-XXX] or WFLY-XXX.

>Will this just be adding a comment or will this also be influencing the status of the pull request - i.e. will these reports be affecting the tick or the cross visible next to a PR?  When looking at the PR queue the ones with the green ticks are the easiest to jump into, provided no changes are required to the code they are potentially ready to merge - adding false crosses would affect this.

I found that status is better way to do this because it doesn't show in the PR log like comments or adding/removing labels do and it's able to demonstrate errors with the cross next to the commit so the author can immediately see that something is wrong and fix it with a few quick actions when the PR is created. Once the status in tyr check is green, assuming no changes to PR structure are made afterwards, it will not influence other statuses posted by CI. 

It boils down to what we really expect from the PR: if prefixing with [WFLY-XXX] or WFLY-XXX is a requirement than we should mandate that every PR has such title. There is also an option to specify when to skip tyr check altogether by skip patterns configurable in PR title, commit message or description (e.g. title with "No issue required"). I can also add an option to disable tyr check by authorized comment from a reviewer if needed.

For sure I don't want rejection of PRs with multiple commits.  It's perfectly valid to write PRs with multiple commits.

There shouldn't be any problem in adjusting this.

Absolutely everything we add to the status list we should be aiming to be accurate, CI is complex and a never ending task to keep on top of intermittent issues and flaky tests but the objective is still to reach that point.  We should not be adding something else to this list deliberately with false negative reviews.

I would say it again depends on what we expect from the PR - it either has a link to the issue or it does not so there is no possibility of false negatives.

I think a key way to keep this accurate is to resist the temptation to have it do more than it should. Don't come up with rules that sound right but turn out to have valid exceptions or that people just won't follow.

Agreed. My initial starting point was PR template but all rules can be adjusted as needed. And this will be configurable anytime so if some rules won't get adopted as expected we can update or remove them.

> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

Martin


On Tue, Apr 9, 2019 at 4:20 PM Brian Stansberry <[hidden email]> wrote:


On Tue, Apr 9, 2019 at 5:39 AM Darran Lofthouse <[hidden email]> wrote:
Overall if for a tool we don't care if it down and consider it's output to be something to ignore it would seem to offer little value, I would suggest we aim for it to be available with useful output.


Yes.
 
Will this just be adding a comment or will this also be influencing the status of the pull request - i.e. will these reports be affecting the tick or the cross visible next to a PR?  When looking at the PR queue the ones with the green ticks are the easiest to jump into, provided no changes are required to the code they are potentially ready to merge - adding false crosses would affect this.

On Tue, Apr 9, 2019 at 6:47 AM Martin Stefanko <[hidden email]> wrote:
Does the commit check run against all commits in the PR?

Not currently, I've added additional checks to verify there is only one commit in the PR :) But this should be possible to do, however, force pushing your way through commit history will not be an easy task.

I think instead it should be checking all commits, it often makes sense to break work into logical chunks - often it is easier to review commit by commit.

For sure I don't want rejection of PRs with multiple commits.  It's perfectly valid to write PRs with multiple commits.

As an example it is a bad practice to bump a subsystem version and add to it within a single commit, GitHub provides no way to diff files like schemas when that happens so the only way to check is to pull the PR locally and manually diff. I suspect the manual diff is a very rare event if it happens at all.  Component upgrades whilst could be in a single commit often make sense to be in individual commits.  When working on an issue and encountering a related but different bug it makes sense to use a separate Jira issue and commit, it is not always practical to submit this fix in it's own PR.  If working on a piece of work and another engineer also follows on you need to ensure you don't modify any shared commits etc... 

Overall the human code review is still mandatory and part of that review would consider the commits and if they make sense.

Yes. 

I see this tool as helping to encourage people to follow proper practice and save humans doing code review having to nag people over such things, or not having the energy to nag, instead accepting things they don't really want.


I think something that could be useful could be to list the checks to be applied somewhere to make it easier to discuss individually, as this would be a change to the WildFly Core and WildFly pull requests it could even make sense to submit as a proposal to the wildfly-proposals repo.
 

What happens if it is down?  I figure nothing really, i.e. the check just isn't performed, which is a quite reasonable failure mode. :) 

AIUI this also has no impact on ability to merge PRs; i.e. we can ignore the result same as we can ignore CI job results. Also good.

Absolutely right. There will be only one additional status in the list. Everything can be ignored.

Absolutely everything we add to the status list we should be aiming to be accurate, CI is complex and a never ending task to keep on top of intermittent issues and flaky tests but the objective is still to reach that point.  We should not be adding something else to this list deliberately with false negative reviews.

Yes, we want it to be accurate. We also want it to be available.

I think a key way to keep this accurate is to resist the temptation to have it do more than it should. Don't come up with rules that sound right but turn out to have valid exceptions or that people just won't follow.

OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.
 
 

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

Ken, I will take this with you offline when it's decided that we can set it up. 

Martin 


On Mon, Apr 8, 2019 at 4:15 PM Ken Wills <[hidden email]> wrote:

On Mon, Apr 8, 2019 at 7:32 AM Martin Stefanko <[hidden email]> wrote:
Yes, understandable. Maybe it can be run where pull-player is being run then? CC Ken.

We can run it in CI somewhere if we need to, I'll need to review the docs and see whats required to run it.

For the initial version, I think we should just try it in OS though, then we can move it at some point in the future.

Ken

 

Martin 


On Mon, Apr 8, 2019 at 12:27 PM Darran Lofthouse <[hidden email]> wrote:
I think if we were to add something to the PR process is should not rely on a single individual to maintain it, if that person is busy, asleep, sick or on holiday then there will be no one to deal with down time and other issues,

On Mon, Apr 8, 2019 at 11:25 AM Martin Stefanko <[hidden email]> wrote:
I can run it my openshift online instance so uptime would be maintained by me for the time being. I have already several instances running like this for some time. During F2F we've also discussed possibility to use Prow project for wildlfy which would require a platform to run (my openshift cannot cover this) and Tyr would nicely integrate with Prow (already tested). However, I am not aware if there has been any movement with Prow setup for wildfly repository recently.

Martin


On Mon, Apr 8, 2019 at 11:26 AM Darran Lofthouse <[hidden email]> wrote:
Couple of questions.

Where does this need to run?

How does this get maintained regarding things like uptime?


On Mon, Apr 8, 2019 at 9:52 AM Martin Stefanko <[hidden email]> wrote:
Hi,

I would like to present my side project Tyr [1] which is a tool that can verify GitHub pull request structure by a YAML definition (example in [2]) provided by a user. The PR author sees the violations immediately in the PR status which allows to correct mistakes right away when the PR is created.

I've recorded a short demo available at [3].

The validation is fully configurable and can be extended with for instance analysis document links and similar.

I would like to include this functionality in wildfly repository. Thoughts?

[3] https://youtu.be/qZRcMQ6qIpg

Martin Stefanko

Software Engineer
Middleware Runtimes Sustaining Engineering Team
Red Hat
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev


--
Brian Stansberry
Manager, Senior Principal Software Engineer
Red Hat

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
> The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.

It would be beneficial to adjust rules as we'll see how useful they are as time progresses. Configuration of rules is external to the service so it will be possible.

Martin 


On Tue, Apr 9, 2019 at 5:49 PM Darran Lofthouse <[hidden email]> wrote:
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
Do we have somewhere yet that we can start to look into an initial set of rules to discuss what the initial set should be?

On Wed, Apr 10, 2019 at 6:32 AM Martin Stefanko <[hidden email]> wrote:
> The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.

It would be beneficial to adjust rules as we'll see how useful they are as time progresses. Configuration of rules is external to the service so it will be possible.

Martin 


On Tue, Apr 9, 2019 at 5:49 PM Darran Lofthouse <[hidden email]> wrote:
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
For the demo I've started with this. For additional checks I have only something to verify that issue in title matches the link in the description. Any ideas are welcome!

I was also thinking about making distinction between issues / bugs and features if that would be useful.


On Wed, Apr 10, 2019 at 11:10 AM Darran Lofthouse <[hidden email]> wrote:
Do we have somewhere yet that we can start to look into an initial set of rules to discuss what the initial set should be?

On Wed, Apr 10, 2019 at 6:32 AM Martin Stefanko <[hidden email]> wrote:
> The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.

It would be beneficial to adjust rules as we'll see how useful they are as time progresses. Configuration of rules is external to the service so it will be possible.

Martin 


On Tue, Apr 9, 2019 at 5:49 PM Darran Lofthouse <[hidden email]> wrote:
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Darran Lofthouse
As an open source project may be better to document somewhere visible to the community.

From the initial rules I would suggest drop the "NO JIRA REQUIRED" from commit messages and PR titles, the commit messages and potentially the titles end up in the history of the project and have no meaning outside of the tool.

At the moment it looks like the template only covers WildFly so may need a second template to cover WildFly Core.

I think from the perspective of the tool making it mandatory for each commit in the PR to contain a Jira ID either with or without the brackets is fine - unless you want to guarantee whitespace around the Jira ID maybe the regular expression could be simplified to ignore the brackets.

Also I think it is reasonable for the tool to expect a link in each PR which should also relate to the project the PR is against.

Regards,
Darran Lofthouse.



On Wed, Apr 10, 2019 at 11:35 AM Martin Stefanko <[hidden email]> wrote:
For the demo I've started with this. For additional checks I have only something to verify that issue in title matches the link in the description. Any ideas are welcome!

I was also thinking about making distinction between issues / bugs and features if that would be useful.


On Wed, Apr 10, 2019 at 11:10 AM Darran Lofthouse <[hidden email]> wrote:
Do we have somewhere yet that we can start to look into an initial set of rules to discuss what the initial set should be?

On Wed, Apr 10, 2019 at 6:32 AM Martin Stefanko <[hidden email]> wrote:
> The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.

It would be beneficial to adjust rules as we'll see how useful they are as time progresses. Configuration of rules is external to the service so it will be possible.

Martin 


On Tue, Apr 9, 2019 at 5:49 PM Darran Lofthouse <[hidden email]> wrote:
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tyr - pull request format checker

Martin Stefanko
Thanks Darran, all valid suggestions. 

I've moved the document to my personal public account [1] - it should be accessible to anyone now.

I will incorporate your suggestions in the gist.


On Wed, Apr 10, 2019 at 12:54 PM Darran Lofthouse <[hidden email]> wrote:
As an open source project may be better to document somewhere visible to the community.

From the initial rules I would suggest drop the "NO JIRA REQUIRED" from commit messages and PR titles, the commit messages and potentially the titles end up in the history of the project and have no meaning outside of the tool.

At the moment it looks like the template only covers WildFly so may need a second template to cover WildFly Core.

I think from the perspective of the tool making it mandatory for each commit in the PR to contain a Jira ID either with or without the brackets is fine - unless you want to guarantee whitespace around the Jira ID maybe the regular expression could be simplified to ignore the brackets.

Also I think it is reasonable for the tool to expect a link in each PR which should also relate to the project the PR is against.

Regards,
Darran Lofthouse.



On Wed, Apr 10, 2019 at 11:35 AM Martin Stefanko <[hidden email]> wrote:
For the demo I've started with this. For additional checks I have only something to verify that issue in title matches the link in the description. Any ideas are welcome!

I was also thinking about making distinction between issues / bugs and features if that would be useful.


On Wed, Apr 10, 2019 at 11:10 AM Darran Lofthouse <[hidden email]> wrote:
Do we have somewhere yet that we can start to look into an initial set of rules to discuss what the initial set should be?

On Wed, Apr 10, 2019 at 6:32 AM Martin Stefanko <[hidden email]> wrote:
> The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.

It would be beneficial to adjust rules as we'll see how useful they are as time progresses. Configuration of rules is external to the service so it will be possible.

Martin 


On Tue, Apr 9, 2019 at 5:49 PM Darran Lofthouse <[hidden email]> wrote:
> OTOH if the mergers decide they want to do something I don't want a tool preventing us doing it, which is what my questions were driving at. We're way smarter than the tool.

The status should not prevent a possibility of merge but if the cross next to the commit would be a problem we can add an override to disable tyr check (meaning make it pass manually) with a comment or similar as suggested above.

The time the cross becomes a problem is if we adopt a set of verification rules that we know will regularly trigger a failure to be ignored, if the failure cases to be ignored are truly exceptional cases then the cross will be useful.
 

Martin



_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev