[IMPORTANT] Procedure around committing pull requests (and commits in general)

[Leif Hedstrom <zw...@apache.org>]

Hi all,

since we’re seeing a pretty significant increase in pull requests (primarily from github), I’d like to remind everyone about some guidelines for committing and testing. This applies to both commits you make on someone else’s pull request, as well as to your own commits.

1. Make sure to review the code, particularly if it’s someone else’s code that you are committing (merging). If you are uncertain, it’s always ok to ask for another pair of eyeballs to take a look. Remember, we do commit then review, and it’s everyones responsibility to review code.

2. Make sure to run all tests before committing. This means at a *minimum*:

	- sudo traffic_server -R 1
	- make test   #from top of build tree

   Neither should fail, both are mandatory to always succeed. For extra bonus and good karma, run tsqa as well (although, that is not as straightfoward as we’d like, yet).

3. Not required, but definitely recommended for large changes: Make a debug build, and run all tests in debug mode. Additionally, I highly recommend everyone has a CentOS6 VM to test builds on, this is our minimum required “platform”.

4. If you are adding new features, or modifying existing features, adding (or modifying) tests is definitely a huge win. Eventually, we might even make it mandatory (but that’s a different topic). 

5. Before you commit, make sure the code is properly formatted using clang-format. This is easiest done with a simple “make clang-format”. Make sure you run / use the correct clang-format binary, from https://bintray.com/apache/trafficserver/clang-format-tools/view <https://bintray.com/apache/trafficserver/clang-format-tools/view> . In addition, there are tools there (such as git clang-format) that are also helpful. I’m working on updated the Docs on the Wiki for these processes as well.

6. Before you commit, check the CI status, at https://ci.trafficserver.apache.org <https://ci.trafficserver.apache.org/>. If there are currently build failures on master, I’d strongly recommend that you defer committing, and instead help fixing the build errors. Even just figuring out what failed, and asking the committer to fix it is a bonus. Piling on more code to an already busted build doesn’t help anyone.

7. Make sure to update any documentation, Jira’s (fix versions, resolutions etc.) and close the github pull request (if applicable).

8. Check your emails and CI for build errors *after* you commit. Emails from Jenkins are flaky at best, so it’s always a good idea to eyeball the site once in a while. It doesn’t take long to get a good idea of what the status is.


Finally, I’d ask everyone to consider joining the issues@trafficserver mailing list, and monitor new and resolved Jira’s, as well as general build errors from Jenkins.

Sincerely,

— leif

Re: [IMPORTANT] Procedure around committing pull requests (and commits in general)

[Thomas Jackson <ja...@gmail.com>]

+1 for automatic tests on PRs :D

We'd have to check with leif on what the jenkins budget looks like, but I
think its probably reasonable to run a subset of the tests on PRs. I'm
thinking:

   - build
   - regression
   - make test
   - tsqa

All on a centos box (since thats the minimum required platform). It should
be significantly cheaper than the full matrix builds we do on master-- and
more importantly should save lots of failed builds on master :)

On Fri, Jul 10, 2015 at 11:36 AM, Bret Palsson <br...@gmail.com> wrote:

> Anyone have a problem with me adding in support to run these tests
> automatically when a pull request comes in? I can leverage Jenkins to do
> this.
>
> -Bret
>
> On Friday, July 10, 2015, James Peach <jp...@apache.org> wrote:
>
> >
> > > On Jul 10, 2015, at 11:21 AM, Leif Hedstrom <zwoop@apache.org
> > <javascript:;>> wrote:
> > >
> > > Hi all,
> > >
> > > since we’re seeing a pretty significant increase in pull requests
> > (primarily from github), I’d like to remind everyone about some
> guidelines
> > for committing and testing. This applies to both commits you make on
> > someone else’s pull request, as well as to your own commits.
> > >
> > > 1. Make sure to review the code, particularly if it’s someone else’s
> > code that you are committing (merging). If you are uncertain, it’s always
> > ok to ask for another pair of eyeballs to take a look. Remember, we do
> > commit then review, and it’s everyones responsibility to review code.
> > >
> > > 2. Make sure to run all tests before committing. This means at a
> > *minimum*:
> > >
> > >       - sudo traffic_server -R 1
> > >       - make test   #from top of build tree
> >
> > The ./ci/regression script does this (as well as verify out-of-tree
> > builds).
> >
> > >   Neither should fail, both are mandatory to always succeed. For extra
> > bonus and good karma, run tsqa as well (although, that is not as
> > straightfoward as we’d like, yet).
> > >
> > > 3. Not required, but definitely recommended for large changes: Make a
> > debug build, and run all tests in debug mode. Additionally, I highly
> > recommend everyone has a CentOS6 VM to test builds on, this is our
> minimum
> > required “platform”.
> > >
> > > 4. If you are adding new features, or modifying existing features,
> > adding (or modifying) tests is definitely a huge win. Eventually, we
> might
> > even make it mandatory (but that’s a different topic).
> > >
> > > 5. Before you commit, make sure the code is properly formatted using
> > clang-format. This is easiest done with a simple “make clang-format”.
> Make
> > sure you run / use the correct clang-format binary, from
> > https://bintray.com/apache/trafficserver/clang-format-tools/view <
> > https://bintray.com/apache/trafficserver/clang-format-tools/view> . In
> > addition, there are tools there (such as git clang-format) that are also
> > helpful. I’m working on updated the Docs on the Wiki for these processes
> as
> > well.
> > >
> > > 6. Before you commit, check the CI status, at
> > https://ci.trafficserver.apache.org <
> https://ci.trafficserver.apache.org/>.
> > If there are currently build failures on master, I’d strongly recommend
> > that you defer committing, and instead help fixing the build errors. Even
> > just figuring out what failed, and asking the committer to fix it is a
> > bonus. Piling on more code to an already busted build doesn’t help
> anyone.
> > >
> > > 7. Make sure to update any documentation, Jira’s (fix versions,
> > resolutions etc.) and close the github pull request (if applicable).
> > >
> > > 8. Check your emails and CI for build errors *after* you commit. Emails
> > from Jenkins are flaky at best, so it’s always a good idea to eyeball the
> > site once in a while. It doesn’t take long to get a good idea of what the
> > status is.
> > >
> > >
> > > Finally, I’d ask everyone to consider joining the issues@trafficserver
> > mailing list, and monitor new and resolved Jira’s, as well as general
> build
> > errors from Jenkins.
> > >
> > > Sincerely,
> > >
> > > — leif
> > >
> >
> >
>
> --
> Bret Palsson | https://cobook.co/bretep
>

Re: [IMPORTANT] Procedure around committing pull requests (and commits in general)

[Bret Palsson <br...@gmail.com>]

Anyone have a problem with me adding in support to run these tests
automatically when a pull request comes in? I can leverage Jenkins to do
this.

-Bret

On Friday, July 10, 2015, James Peach <jp...@apache.org> wrote:

>
> > On Jul 10, 2015, at 11:21 AM, Leif Hedstrom <zwoop@apache.org
> <javascript:;>> wrote:
> >
> > Hi all,
> >
> > since we’re seeing a pretty significant increase in pull requests
> (primarily from github), I’d like to remind everyone about some guidelines
> for committing and testing. This applies to both commits you make on
> someone else’s pull request, as well as to your own commits.
> >
> > 1. Make sure to review the code, particularly if it’s someone else’s
> code that you are committing (merging). If you are uncertain, it’s always
> ok to ask for another pair of eyeballs to take a look. Remember, we do
> commit then review, and it’s everyones responsibility to review code.
> >
> > 2. Make sure to run all tests before committing. This means at a
> *minimum*:
> >
> >       - sudo traffic_server -R 1
> >       - make test   #from top of build tree
>
> The ./ci/regression script does this (as well as verify out-of-tree
> builds).
>
> >   Neither should fail, both are mandatory to always succeed. For extra
> bonus and good karma, run tsqa as well (although, that is not as
> straightfoward as we’d like, yet).
> >
> > 3. Not required, but definitely recommended for large changes: Make a
> debug build, and run all tests in debug mode. Additionally, I highly
> recommend everyone has a CentOS6 VM to test builds on, this is our minimum
> required “platform”.
> >
> > 4. If you are adding new features, or modifying existing features,
> adding (or modifying) tests is definitely a huge win. Eventually, we might
> even make it mandatory (but that’s a different topic).
> >
> > 5. Before you commit, make sure the code is properly formatted using
> clang-format. This is easiest done with a simple “make clang-format”. Make
> sure you run / use the correct clang-format binary, from
> https://bintray.com/apache/trafficserver/clang-format-tools/view <
> https://bintray.com/apache/trafficserver/clang-format-tools/view> . In
> addition, there are tools there (such as git clang-format) that are also
> helpful. I’m working on updated the Docs on the Wiki for these processes as
> well.
> >
> > 6. Before you commit, check the CI status, at
> https://ci.trafficserver.apache.org <https://ci.trafficserver.apache.org/>.
> If there are currently build failures on master, I’d strongly recommend
> that you defer committing, and instead help fixing the build errors. Even
> just figuring out what failed, and asking the committer to fix it is a
> bonus. Piling on more code to an already busted build doesn’t help anyone.
> >
> > 7. Make sure to update any documentation, Jira’s (fix versions,
> resolutions etc.) and close the github pull request (if applicable).
> >
> > 8. Check your emails and CI for build errors *after* you commit. Emails
> from Jenkins are flaky at best, so it’s always a good idea to eyeball the
> site once in a while. It doesn’t take long to get a good idea of what the
> status is.
> >
> >
> > Finally, I’d ask everyone to consider joining the issues@trafficserver
> mailing list, and monitor new and resolved Jira’s, as well as general build
> errors from Jenkins.
> >
> > Sincerely,
> >
> > — leif
> >
>
>

-- 
Bret Palsson | https://cobook.co/bretep

Re: [IMPORTANT] Procedure around committing pull requests (and commits in general)

[James Peach <jp...@apache.org>]

> On Jul 10, 2015, at 11:21 AM, Leif Hedstrom <zw...@apache.org> wrote:
> 
> Hi all,
> 
> since we’re seeing a pretty significant increase in pull requests (primarily from github), I’d like to remind everyone about some guidelines for committing and testing. This applies to both commits you make on someone else’s pull request, as well as to your own commits.
> 
> 1. Make sure to review the code, particularly if it’s someone else’s code that you are committing (merging). If you are uncertain, it’s always ok to ask for another pair of eyeballs to take a look. Remember, we do commit then review, and it’s everyones responsibility to review code.
> 
> 2. Make sure to run all tests before committing. This means at a *minimum*:
> 
> 	- sudo traffic_server -R 1
> 	- make test   #from top of build tree

The ./ci/regression script does this (as well as verify out-of-tree builds).

>   Neither should fail, both are mandatory to always succeed. For extra bonus and good karma, run tsqa as well (although, that is not as straightfoward as we’d like, yet).
> 
> 3. Not required, but definitely recommended for large changes: Make a debug build, and run all tests in debug mode. Additionally, I highly recommend everyone has a CentOS6 VM to test builds on, this is our minimum required “platform”.
> 
> 4. If you are adding new features, or modifying existing features, adding (or modifying) tests is definitely a huge win. Eventually, we might even make it mandatory (but that’s a different topic). 
> 
> 5. Before you commit, make sure the code is properly formatted using clang-format. This is easiest done with a simple “make clang-format”. Make sure you run / use the correct clang-format binary, from https://bintray.com/apache/trafficserver/clang-format-tools/view <https://bintray.com/apache/trafficserver/clang-format-tools/view> . In addition, there are tools there (such as git clang-format) that are also helpful. I’m working on updated the Docs on the Wiki for these processes as well.
> 
> 6. Before you commit, check the CI status, at https://ci.trafficserver.apache.org <https://ci.trafficserver.apache.org/>. If there are currently build failures on master, I’d strongly recommend that you defer committing, and instead help fixing the build errors. Even just figuring out what failed, and asking the committer to fix it is a bonus. Piling on more code to an already busted build doesn’t help anyone.
> 
> 7. Make sure to update any documentation, Jira’s (fix versions, resolutions etc.) and close the github pull request (if applicable).
> 
> 8. Check your emails and CI for build errors *after* you commit. Emails from Jenkins are flaky at best, so it’s always a good idea to eyeball the site once in a while. It doesn’t take long to get a good idea of what the status is.
> 
> 
> Finally, I’d ask everyone to consider joining the issues@trafficserver mailing list, and monitor new and resolved Jira’s, as well as general build errors from Jenkins.
> 
> Sincerely,
> 
> — leif
>