[ovs-dev,RFC] OVS robot discussion - check API integration
diff mbox series

Message ID f7th8daqrfy.fsf@dhcp-25.97.bos.redhat.com
State RFC
Headers show
Series
  • [ovs-dev,RFC] OVS robot discussion - check API integration
Related show

Commit Message

Aaron Conole Feb. 11, 2019, 3:24 p.m. UTC
I'm following up on a discussion where we briefly talked about adding
a 'check' to OVS patch series during the OVS Conference.  'Check' is
what the patchwork project provides to show that certain tests have been
performed.  It is made up of a name, a status, and a URL.  Some of the
projects in the ozlabs patchwork instance are using the check feature
already, so there's some examples to reference.

My idea is to push some data into travis / cirrus that can be used to
insert a check against a patch series.  This would allow committers to
look at the status page on patchwork and see at a glance the links to
the travis, and if a robot email was sent.

The naivest way I can think of getting these results published is by
modifying ${OS}-build.sh like:

---
---

I think this could work.  We can use the travis "encrypted info" data to
push the credentials but that probably needs to be encrypted for the
robot key.  It does put some kind of pollution in the travis / cirrus
builds (because 'series_*' appears nowhere in the official repository).
Maybe that isn't acceptable.

I am planning to follow the guidebook at
https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings
and store the credentials variables for just the OVS robot repo.

Maybe there's another opinion.  Probably there's a better way of
sending the data around, sharing credentials, or passing requests so
that other projects can also integrate.  Thoughts, opinions, comments?

Comments

Ben Pfaff Feb. 11, 2019, 5:03 p.m. UTC | #1
On Mon, Feb 11, 2019 at 10:24:01AM -0500, Aaron Conole wrote:
> I'm following up on a discussion where we briefly talked about adding
> a 'check' to OVS patch series during the OVS Conference.  'Check' is
> what the patchwork project provides to show that certain tests have been
> performed.  It is made up of a name, a status, and a URL.  Some of the
> projects in the ozlabs patchwork instance are using the check feature
> already, so there's some examples to reference.
> 
> My idea is to push some data into travis / cirrus that can be used to
> insert a check against a patch series.  This would allow committers to
> look at the status page on patchwork and see at a glance the links to
> the travis, and if a robot email was sent.

The approach seems reasonable.

You mention that there are existing examples.  How do they do it?
Thomas Monjalon Feb. 11, 2019, 5:40 p.m. UTC | #2
11/02/2019 18:03, Ben Pfaff:
> On Mon, Feb 11, 2019 at 10:24:01AM -0500, Aaron Conole wrote:
> > I'm following up on a discussion where we briefly talked about adding
> > a 'check' to OVS patch series during the OVS Conference.  'Check' is
> > what the patchwork project provides to show that certain tests have been
> > performed.  It is made up of a name, a status, and a URL.  Some of the
> > projects in the ozlabs patchwork instance are using the check feature
> > already, so there's some examples to reference.
> > 
> > My idea is to push some data into travis / cirrus that can be used to
> > insert a check against a patch series.  This would allow committers to
> > look at the status page on patchwork and see at a glance the links to
> > the travis, and if a robot email was sent.
> 
> The approach seems reasonable.
> 
> You mention that there are existing examples.  How do they do it?

In DPDK project, we are sending the results to a specific mailing-list
with this script:
	http://git.dpdk.org/tools/dpdk-ci/tree/tools/send-patch-report.sh
It is possible to filter some reports by using mail filtering.
Then it is parsed on the server by the following script which updates
patchwork data using server-specific credentials:
	http://git.dpdk.org/tools/dpdk-ci/tree/tools/update-pw.sh

Note that we improved patchwork to highlight CI results with colors:
	https://patches.dpdk.org
Ilya Maximets Feb. 13, 2019, 12:45 p.m. UTC | #3
On 11.02.2019 18:24, Aaron Conole wrote:
> I'm following up on a discussion where we briefly talked about adding
> a 'check' to OVS patch series during the OVS Conference.  'Check' is
> what the patchwork project provides to show that certain tests have been
> performed.  It is made up of a name, a status, and a URL.  Some of the
> projects in the ozlabs patchwork instance are using the check feature
> already, so there's some examples to reference.
> 
> My idea is to push some data into travis / cirrus that can be used to
> insert a check against a patch series.  This would allow committers to
> look at the status page on patchwork and see at a glance the links to
> the travis, and if a robot email was sent.
> 
> The naivest way I can think of getting these results published is by
> modifying ${OS}-build.sh like:
> 
> ---
> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> index 0a2091061..38359c86f 100755
> --- a/.travis/linux-build.sh
> +++ b/.travis/linux-build.sh
> @@ -8,6 +8,47 @@ SPARSE_FLAGS=""
>  EXTRA_OPTS=""
>  TARGET="x86_64-native-linuxapp-gcc"
>  
> +function start_patchwork_check()
> +{
> +    if [ ! -e ".git/config" ]; then
> +        return;
> +    fi
> +
> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
> +        return;
> +    fi
> +
> +    curl -s $PW_API_URL/$(SERIES_ID) ...submit check start ...
> +}
> +
> +function bad_check_result()
> +{
> +    if [ ! -e ".git/config" ]; then
> +        return;
> +    fi
> +
> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
> +        return;
> +    fi
> +
> +    curl -s $PW_API_URL/$(SERIES_ID) ...set check to bad...
> +}
> +
> +function check_success()
> +{
> +    if [ ! -e ".git/config" ]; then
> +        return;
> +    fi
> +
> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
> +        return;
> +    fi
> +
> +    curl -s $PW_API_URL/$(SERIES_ID) ...set check to success...
> +}
> +
> +trap '[ "$?" -eq 0 ] || bad_check_result' INT ERR TERM
> +
>  function install_kernel()
>  {
>      if [[ "$1" =~ ^4.* ]]; then
> ---
> 
> I think this could work.  We can use the travis "encrypted info" data to
> push the credentials but that probably needs to be encrypted for the
> robot key.  It does put some kind of pollution in the travis / cirrus
> builds (because 'series_*' appears nowhere in the official repository).
> Maybe that isn't acceptable.
> 
> I am planning to follow the guidebook at
> https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings
> and store the credentials variables for just the OVS robot repo.
> 
> Maybe there's another opinion.  Probably there's a better way of
> sending the data around, sharing credentials, or passing requests so
> that other projects can also integrate.  Thoughts, opinions, comments?

I don't know if it could be an issue, but I just wanted to warn about
'set -x' that we have now in a Travis scripts. You need to be careful
with exported credentials.

And we, actually, already have special ovs-build mail-list. It could
be used if you'll decide to choose DPDK way of working with patchwork.

Best regards, Ilya Maximets.
Aaron Conole Feb. 13, 2019, 4:56 p.m. UTC | #4
Ilya Maximets <i.maximets@samsung.com> writes:

> On 11.02.2019 18:24, Aaron Conole wrote:
>> I'm following up on a discussion where we briefly talked about adding
>> a 'check' to OVS patch series during the OVS Conference.  'Check' is
>> what the patchwork project provides to show that certain tests have been
>> performed.  It is made up of a name, a status, and a URL.  Some of the
>> projects in the ozlabs patchwork instance are using the check feature
>> already, so there's some examples to reference.
>> 
>> My idea is to push some data into travis / cirrus that can be used to
>> insert a check against a patch series.  This would allow committers to
>> look at the status page on patchwork and see at a glance the links to
>> the travis, and if a robot email was sent.
>> 
>> The naivest way I can think of getting these results published is by
>> modifying ${OS}-build.sh like:
>> 
>> ---
>> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
>> index 0a2091061..38359c86f 100755
>> --- a/.travis/linux-build.sh
>> +++ b/.travis/linux-build.sh
>> @@ -8,6 +8,47 @@ SPARSE_FLAGS=""
>>  EXTRA_OPTS=""
>>  TARGET="x86_64-native-linuxapp-gcc"
>>  
>> +function start_patchwork_check()
>> +{
>> +    if [ ! -e ".git/config" ]; then
>> +        return;
>> +    fi
>> +
>> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
>> +        return;
>> +    fi
>> +
>> +    curl -s $PW_API_URL/$(SERIES_ID) ...submit check start ...
>> +}
>> +
>> +function bad_check_result()
>> +{
>> +    if [ ! -e ".git/config" ]; then
>> +        return;
>> +    fi
>> +
>> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
>> +        return;
>> +    fi
>> +
>> +    curl -s $PW_API_URL/$(SERIES_ID) ...set check to bad...
>> +}
>> +
>> +function check_success()
>> +{
>> +    if [ ! -e ".git/config" ]; then
>> +        return;
>> +    fi
>> +
>> +    if ! git branch | grep \\* | grep series_ >/dev/null; then
>> +        return;
>> +    fi
>> +
>> +    curl -s $PW_API_URL/$(SERIES_ID) ...set check to success...
>> +}
>> +
>> +trap '[ "$?" -eq 0 ] || bad_check_result' INT ERR TERM
>> +
>>  function install_kernel()
>>  {
>>      if [[ "$1" =~ ^4.* ]]; then
>> ---
>> 
>> I think this could work.  We can use the travis "encrypted info" data to
>> push the credentials but that probably needs to be encrypted for the
>> robot key.  It does put some kind of pollution in the travis / cirrus
>> builds (because 'series_*' appears nowhere in the official repository).
>> Maybe that isn't acceptable.
>> 
>> I am planning to follow the guidebook at
>> https://docs.travis-ci.com/user/environment-variables/#defining-variables-in-repository-settings
>> and store the credentials variables for just the OVS robot repo.
>> 
>> Maybe there's another opinion.  Probably there's a better way of
>> sending the data around, sharing credentials, or passing requests so
>> that other projects can also integrate.  Thoughts, opinions, comments?
>
> I don't know if it could be an issue, but I just wanted to warn about
> 'set -x' that we have now in a Travis scripts. You need to be careful
> with exported credentials.

Keen observation!

> And we, actually, already have special ovs-build mail-list. It could
> be used if you'll decide to choose DPDK way of working with patchwork.

It looks intriguing.  I like the idea that anyone could then contribute
'check results' to the patchwork project.  But, it seems like it would
require some additional infrastructure.  We would need a script that
will periodically run against the list, or possibly an alias that is
subscribed to the list that is just an alias to
/bin/process_ovs_build_mails.sh that can update the check interface.

> Best regards, Ilya Maximets.
Thomas Monjalon Feb. 13, 2019, 5:35 p.m. UTC | #5
13/02/2019 17:56, Aaron Conole:
> Ilya Maximets <i.maximets@samsung.com> writes:
> > And we, actually, already have special ovs-build mail-list. It could
> > be used if you'll decide to choose DPDK way of working with patchwork.
> 
> It looks intriguing.  I like the idea that anyone could then contribute
> 'check results' to the patchwork project.  But, it seems like it would
> require some additional infrastructure.  We would need a script that
> will periodically run against the list, or possibly an alias that is
> subscribed to the list that is just an alias to
> /bin/process_ovs_build_mails.sh that can update the check interface.

The best is to have a hook in the mail archiving system,
so you can reference the report in the mail archives
as soon as it is published.
Aaron Conole Feb. 25, 2019, 6:42 p.m. UTC | #6
Thomas Monjalon <thomas@monjalon.net> writes:

> 13/02/2019 17:56, Aaron Conole:
>> Ilya Maximets <i.maximets@samsung.com> writes:
>> > And we, actually, already have special ovs-build mail-list. It could
>> > be used if you'll decide to choose DPDK way of working with patchwork.
>> 
>> It looks intriguing.  I like the idea that anyone could then contribute
>> 'check results' to the patchwork project.  But, it seems like it would
>> require some additional infrastructure.  We would need a script that
>> will periodically run against the list, or possibly an alias that is
>> subscribed to the list that is just an alias to
>> /bin/process_ovs_build_mails.sh that can update the check interface.
>
> The best is to have a hook in the mail archiving system,
> so you can reference the report in the mail archives
> as soon as it is published.

I'm not sure whether anyone can run such a script on the ovs mail
server.  Regardless, if there is something subscribed to the list, we
can probably scan the list archives for the appropriate url and update
with that link.
Justin Pettit Feb. 25, 2019, 10:51 p.m. UTC | #7
> On Feb 25, 2019, at 10:42 AM, Aaron Conole <aconole@redhat.com> wrote:
> 
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
>> The best is to have a hook in the mail archiving system,
>> so you can reference the report in the mail archives
>> as soon as it is published.
> 
> I'm not sure whether anyone can run such a script on the ovs mail
> server.  Regardless, if there is something subscribed to the list, we
> can probably scan the list archives for the appropriate url and update
> with that link.

Right.  Linux Foundation runs our mail server, so this would probably be a hard sell.  :-)  If we could run something externally, I imagine that will be easier.

--Justin

Patch
diff mbox series

diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index 0a2091061..38359c86f 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -8,6 +8,47 @@  SPARSE_FLAGS=""
 EXTRA_OPTS=""
 TARGET="x86_64-native-linuxapp-gcc"
 
+function start_patchwork_check()
+{
+    if [ ! -e ".git/config" ]; then
+        return;
+    fi
+
+    if ! git branch | grep \\* | grep series_ >/dev/null; then
+        return;
+    fi
+
+    curl -s $PW_API_URL/$(SERIES_ID) ...submit check start ...
+}
+
+function bad_check_result()
+{
+    if [ ! -e ".git/config" ]; then
+        return;
+    fi
+
+    if ! git branch | grep \\* | grep series_ >/dev/null; then
+        return;
+    fi
+
+    curl -s $PW_API_URL/$(SERIES_ID) ...set check to bad...
+}
+
+function check_success()
+{
+    if [ ! -e ".git/config" ]; then
+        return;
+    fi
+
+    if ! git branch | grep \\* | grep series_ >/dev/null; then
+        return;
+    fi
+
+    curl -s $PW_API_URL/$(SERIES_ID) ...set check to success...
+}
+
+trap '[ "$?" -eq 0 ] || bad_check_result' INT ERR TERM
+
 function install_kernel()
 {
     if [[ "$1" =~ ^4.* ]]; then