Test failures and merging PRs

Development
1

Hello devs,

since there was a discussion on foreman-dev IRC channel recently about merging
PRs in Foreman core even if there's some build failed, I talked to few people
and decided to describe here what I think is current way of how it works. I'm
also attaching one suggestion at the end that came up after the discussion.

Please, add questions, comments or simple +1 so we all know whether we're on
the same page.

Core PR runs 7 checks - foreman, katello, codeclimate, hound, prprocessor,
upgrade, continuous-integration/travis-ci/pr. In ideal case they are all green
and after review, the PR is merged. There are several cases where we can merge
even if the PR is red.

  1. codeclimate is red

This can be ignored, we never agreed on using this as a hard metric for the
PR. The motivation to introduce it was mainly to save some time to reviewer.
We don't have to run it manually to get indications whether there's something
introducing a big complexity [1]. From my experience it sometimes leads to
worse code, since author splits the logic into more methods to lower e.g.
cyclomatic complexity but it should be judged separately in every case.

  1. foreman is red

This can happen because of intermittent tests failures. If the PR is clearly
not causing new ones and commiter is aware of this error, the PR is merged
with message like "test unrelated" comments. If we are not sure, we retrigger
the run,

If Foreman develop branch is broken, we need to merge the PR to fix it so this
is another exception. Usually we trigger the jenkins job manually first to see
that the PR fixes the issue.

  1. katello is red

If katello becomes red only for this PR, we analyze what causes it. Usually it
means that we change some internal things that have impact on Katello. In such
case, we're doing our best to send a fixing PR to Katello or we ping someone
with better knowledge in this area. We don't merge the PR until it's resolved,
then usually we merge both parts at the same time.

If it turns out there are more PRs that are failing with same errors, we merge
PRs if we're sure they don't introduce new Katello failures. At this time,
we're not blocking merges until Katello is green again. (*) here the
suggestion applies

  1. upgrade is red

this is very similar to katello job, if there's some issue in upgrade, we
should not merge the PR. I remember though, there was a time when we knew the
job is broken which fall under "known to be broken" category.

  1. There's no 5, all the rest must be green. Sometimes hound service does not
    respond and remains in "running" state, then it's retriggered by the reviewer.
    prprocessor and travis must always be happy.

Now the promised suggestion. When we see katello/upgrade job is broken on
multiple PRs, the first reviewer who spots it should notify the rest of
developers about "from now on, we ignore tests XY". The ideal channel seems to
be this list. This helps Katello devs to find out when it started, what were
the commits that first induced it.

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

··· -- Marek
2

> Hello devs,
>
> since there was a discussion on foreman-dev IRC channel recently about merging
> PRs in Foreman core even if there's some build failed, I talked to few people
> and decided to describe here what I think is current way of how it works. I'm
> also attaching one suggestion at the end that came up after the discussion.
>
> Please, add questions, comments or simple +1 so we all know whether we're on
> the same page.
>
> Core PR runs 7 checks - foreman, katello, codeclimate, hound, prprocessor,
> upgrade, continuous-integration/travis-ci/pr. In ideal case they are all green
> and after review, the PR is merged. There are several cases where we can merge
> even if the PR is red.
>
> 1) codeclimate is red
>
> This can be ignored, we never agreed on using this as a hard metric for the
> PR. The motivation to introduce it was mainly to save some time to reviewer.
> We don't have to run it manually to get indications whether there's something
> introducing a big complexity [1]. From my experience it sometimes leads to
> worse code, since author splits the logic into more methods to lower e.g.
> cyclomatic complexity but it should be judged separately in every case.

It should be taken care of whenever possible. Most of the times it's
certainly right and notices typical problems like stupidly long classes,
duplication, etc…

https://codeclimate.com/github/rails/rails itself enforces it unless a
maintainer vets the PR. (ad hominem fallacy, I know)

>
> 2) foreman is red
>
> This can happen because of intermittent tests failures. If the PR is clearly
> not causing new ones and commiter is aware of this error, the PR is merged
> with message like "test unrelated" comments. If we are not sure, we retrigger
> the run,
>
> If Foreman develop branch is broken, we need to merge the PR to fix it so this
> is another exception. Usually we trigger the jenkins job manually first to see
> that the PR fixes the issue.

Makes sense to me, generally. The only con I've seen since I started
contributing is that few people care to fix intermittent tests, which
caused the job to be red for weeks at times

>
> 3) katello is red
>
> If katello becomes red only for this PR, we analyze what causes it. Usually it
> means that we change some internal things that have impact on Katello. In such
> case, we're doing our best to send a fixing PR to Katello or we ping someone
> with better knowledge in this area. We don't merge the PR until it's resolved,
> then usually we merge both parts at the same time.

This sounds good. Ideally the contributor sends the patch to Katello, please.
>
> If it turns out there are more PRs that are failing with same errors, we merge
> PRs if we're sure they don't introduce new Katello failures. At this time,
> we're not blocking merges until Katello is green again. (*) here the
> suggestion applies

I don't think this is fair. If the Katello job is red, it's our
responsibility to bring it back to green. When the causes for the job to
be red are unknown, merging more stuff in Foreman will certainly NOT
make it easier to understand them. In fact it may just aggravate the
problem. So I would say no - at least on PRs I'm reviewing, I'm not
merging if Katello is red.

>
> 4) upgrade is red
>
> this is very similar to katello job, if there's some issue in upgrade, we
> should not merge the PR. I remember though, there was a time when we knew the
> job is broken which fall under "known to be broken" category.

Same as 3.

>
> 5) There's no 5, all the rest must be green. Sometimes hound service does not
> respond and remains in "running" state, then it's retriggered by the reviewer.
> prprocessor and travis must always be happy.
>
> Now the promised suggestion. When we see katello/upgrade job is broken on
> multiple PRs, the first reviewer who spots it should notify the rest of
> developers about "from now on, we ignore tests XY". The ideal channel seems to
> be this list. This helps Katello devs to find out when it started, what were
> the commits that first induced it.

Sorry, I don't think this is a better idea than the current (unspoken)
approach to block PRs until Katello is merged. It's good you started
this thread, as clearly people have different assumptions on this topic
as we didn't talk about it much.

If we reach to some conclusions, updating
Foreman :: Contribute with them would be good to have
some reference set in stone (or in bytes (: ).

··· On 08/28, Marek Hulán wrote:

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

–
Marek

–
You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
Daniel Lobato Garcia

@dLobatog
blog.daniellobato.me
daniellobato.me

GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
Keybase: https://keybase.io/elobato

3

> 1) codeclimate is red
>
> This can be ignored, we never agreed on using this as a hard metric for the
> PR. The motivation to introduce it was mainly to save some time to reviewer.
> We don't have to run it manually to get indications whether there's something
> introducing a big complexity [1]. From my experience it sometimes leads to
> worse code, since author splits the logic into more methods to lower e.g.
> cyclomatic complexity but it should be judged separately in every case.
+1
I like it as a suggestion, but sometimes it's just off and better be
ignored.
>
> 2) foreman is red
>
> This can happen because of intermittent tests failures. If the PR is clearly
> not causing new ones and commiter is aware of this error, the PR is merged
> with message like "test unrelated" comments. If we are not sure, we retrigger
> the run,
>
> If Foreman develop branch is broken, we need to merge the PR to fix it so this
> is another exception. Usually we trigger the jenkins job manually first to see
> that the PR fixes the issue.
+1
Yes, don't merge a PR with failing Foreman core tests.
>
> 3) katello is red
>
> If katello becomes red only for this PR, we analyze what causes it. Usually it
> means that we change some internal things that have impact on Katello. In such
> case, we're doing our best to send a fixing PR to Katello or we ping someone
> with better knowledge in this area. We don't merge the PR until it's resolved,
> then usually we merge both parts at the same time.
I think, this is totally unfair to all the "smaller" plugin maintainers
and that's why I vote for removing the test completely or just keep it
to test our public APIs.
I believe, we should do the following:
If the Foreman PR breaks some public API, e.g. facets, and the Katello
tests show that, my suggestion is to fix the foreman PR to not break the
public API and add proper depreciations if possible.
If we change something inside Foreman - in the past we changed the host
multiple actions from GET to POST or introduced strong parameters for
example - the contributor or maintainer should send a mail to
foreman-dev expaining what needs to be changed in plugins. I think it's
also a good idea to fix the example plugin or the How to create a plugin
wiki page if applicable.
However I think, it's the plugin maintainer's responsibility to make
sure his plugin works with Foreman. Everything else doesn't scale. In
the past a lot of "my" plugins broke because of changes to Foreman core.
Nobody cared to send a PR so far. But that's fine. I don't expect
anybody to. It's my job to test the plugin and fix it if it breaks.
I think, we should not block Foreman PRs because an additional parameter
was added to some internal method, just because Katello happens to
overwrite that method. It just doesn't make any sense to me why we
should do that for Katello but not for all the other plugins out there.
This is not against Katello, it's just the only plugin tested with
Foreman core right now.
Currently, we're developing a plugin to show system logfiles in Foreman.
That requires a complete ELK-stack for development. I would not expect
every developer to have that at hand.
If we leave Katello in the matrix, I think it would be totally fair to
also add our new plugin to Foreman's test matrix as well. But I wouldn't
want to block Foreman development just because some test in that plugin
breaks.
I know, Katello is important for RedHat and it's one of the larger
plugins. But that doesn't justify a special role in my opinion.
> If it turns out there are more PRs that are failing with same errors, we merge
> PRs if we're sure they don't introduce new Katello failures. At this time,
> we're not blocking merges until Katello is green again. (*) here the
> suggestion applies
>
> 4) upgrade is red
>
> this is very similar to katello job, if there's some issue in upgrade, we
> should not merge the PR. I remember though, there was a time when we knew the
> job is broken which fall under "known to be broken" category.
+1, if upgrade is broken the PR is broken.
>
> 5) There's no 5, all the rest must be green. Sometimes hound service does not
> respond and remains in "running" state, then it's retriggered by the reviewer.
> prprocessor and travis must always be happy.
Don't get me started on hound. But it's the best we have right now, so
generally speaking: Yes: Linting is important. If there are lint
warnings, we shouldn't merge the PR.
>
> Now the promised suggestion. When we see katello/upgrade job is broken on
> multiple PRs, the first reviewer who spots it should notify the rest of
> developers about "from now on, we ignore tests XY". The ideal channel seems to
> be this list. This helps Katello devs to find out when it started, what were
> the commits that first induced it.
>
> [1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk
>
> Thanks for comments
>
> –
> Marek
>
Timo

··· Am 28.08.17 um 17:12 schrieb Marek Hulán:
4

>
>> 1) codeclimate is red
>>
>> This can be ignored, we never agreed on using this as a hard metric for
>> the
>> PR. The motivation to introduce it was mainly to save some time to
>> reviewer.
>> We don't have to run it manually to get indications whether there's
>> something
>> introducing a big complexity [1]. From my experience it sometimes leads to
>> worse code, since author splits the logic into more methods to lower e.g.
>> cyclomatic complexity but it should be judged separately in every case.
>>
> +1
> I like it as a suggestion, but sometimes it's just off and better be
> ignored.
>
>>
>> 2) foreman is red
>>
>> This can happen because of intermittent tests failures. If the PR is
>> clearly
>> not causing new ones and commiter is aware of this error, the PR is merged
>> with message like "test unrelated" comments. If we are not sure, we
>> retrigger
>> the run,
>>
>> If Foreman develop branch is broken, we need to merge the PR to fix it so
>> this
>> is another exception. Usually we trigger the jenkins job manually first
>> to see
>> that the PR fixes the issue.
>>
> +1
> Yes, don't merge a PR with failing Foreman core tests.
>
>>
>> 3) katello is red
>>
>> If katello becomes red only for this PR, we analyze what causes it.
>> Usually it
>> means that we change some internal things that have impact on Katello. In
>> such
>> case, we're doing our best to send a fixing PR to Katello or we ping
>> someone
>> with better knowledge in this area. We don't merge the PR until it's
>> resolved,
>> then usually we merge both parts at the same time.
>>
> I think, this is totally unfair to all the "smaller" plugin maintainers
> and that's why I vote for removing the test completely or just keep it to
> test our public APIs.
> I believe, we should do the following:
> If the Foreman PR breaks some public API, e.g. facets, and the Katello
> tests show that, my suggestion is to fix the foreman PR to not break the
> public API and add proper depreciations if possible.
> If we change something inside Foreman - in the past we changed the host
> multiple actions from GET to POST or introduced strong parameters for
> example - the contributor or maintainer should send a mail to foreman-dev
> expaining what needs to be changed in plugins. I think it's also a good
> idea to fix the example plugin or the How to create a plugin wiki page if
> applicable.
> However I think, it's the plugin maintainer's responsibility to make sure
> his plugin works with Foreman. Everything else doesn't scale. In the past a
> lot of "my" plugins broke because of changes to Foreman core. Nobody cared
> to send a PR so far. But that's fine. I don't expect anybody to. It's my
> job to test the plugin and fix it if it breaks.
> I think, we should not block Foreman PRs because an additional parameter
> was added to some internal method, just because Katello happens to
> overwrite that method. It just doesn't make any sense to me why we should
> do that for Katello but not for all the other plugins out there. This is
> not against Katello, it's just the only plugin tested with Foreman core
> right now.
> Currently, we're developing a plugin to show system logfiles in Foreman.
> That requires a complete ELK-stack for development. I would not expect
> every developer to have that at hand.
> If we leave Katello in the matrix, I think it would be totally fair to
> also add our new plugin to Foreman's test matrix as well. But I wouldn't
> want to block Foreman development just because some test in that plugin
> breaks.
> I know, Katello is important for RedHat and it's one of the larger
> plugins. But that doesn't justify a special role in my opinion.

I'm going to try to approach this from an unbiased position, so if I start
to lean a particular way please forgive me up front and try to ignore the
leaning. I think it's important for us to look at the core reasoning behind
why this set of tests was added to Foreman PRs and agreed upon by Foreman
and Katello to begin with.

  1. Reduce developer inefficiency by preventing broken environments
  2. Reduce unplanned work n the form of drop everything fixes

With Katello being a large plugin no only in scope, but in developer size
whenever a breakage would occur a large set of developers would end up with
broken environments that required 1-2 developers to drop what they were
doing to fix the issue immediately or face a growing backlog of PRs
followed by build breakages which has resulted in a snowball effect. This
has never been an enjoyable position to be in, since it could come on
unexpectedly. Other plugin's have suffered the same and expressed the same.

I would argue that since Foreman provides the plugin ability, it has to
honor not breaking them. When this all started, there was a shallow plugin
API and the way to "integrate" your plugin was through judicious use of
Rubyisms not a public, standardized API. Times have changed and we should
change with them. However, I don't think we can whole hog change the way
things work until we provide the ability to do things properly. To that
end, I think we have two other conversations worth having and gathering
data about:

  1. What does it mean to be a plugin
  2. What are the ways plugins try to extend Foreman and which are not
    covered by a sustainable API

To date, the best resource we have is [1]. And note, even [1] describes
methods of overriding any functionality found within Foreman (or another
plugin for that matter). This could both easily be their own email topics
or a single topic but moved to their own thread. I've also wondered if
plugins are still the right way to go if we stopped to look back at what
our goals with them are but thats another, much larger discussion to be had.

I am happy to break out these topics to their own threads if people think
that would be prudent.

Eric

[1]
http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin

··· On Thu, Aug 31, 2017 at 5:08 PM, Timo Goebel wrote: > Am 28.08.17 um 17:12 schrieb Marek Hulán:

If it turns out there are more PRs that are failing with same errors, we

merge
PRs if we’re sure they don’t introduce new Katello failures. At this time,
we’re not blocking merges until Katello is green again. (*) here the
suggestion applies

  1. upgrade is red

this is very similar to katello job, if there’s some issue in upgrade, we
should not merge the PR. I remember though, there was a time when we knew
the
job is broken which fall under “known to be broken” category.

+1, if upgrade is broken the PR is broken.

  1. There’s no 5, all the rest must be green. Sometimes hound service does
    not
    respond and remains in “running” state, then it’s retriggered by the
    reviewer.
    prprocessor and travis must always be happy.

Don’t get me started on hound. But it’s the best we have right now, so
generally speaking: Yes: Linting is important. If there are lint warnings,
we shouldn’t merge the PR.

Now the promised suggestion. When we see katello/upgrade job is broken on
multiple PRs, the first reviewer who spots it should notify the rest of
developers about “from now on, we ignore tests XY”. The ideal channel
seems to
be this list. This helps Katello devs to find out when it started, what
were
the commits that first induced it.

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

–
Marek

Timo

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
Eric D. Helms
Red Hat Engineering

5

Thanks Timo for your input. Please see my comment below in the text.

> > 1) codeclimate is red
> >
> > This can be ignored, we never agreed on using this as a hard metric for
> > the
> > PR. The motivation to introduce it was mainly to save some time to
> > reviewer. We don't have to run it manually to get indications whether
> > there's something introducing a big complexity [1]. From my experience it
> > sometimes leads to worse code, since author splits the logic into more
> > methods to lower e.g. cyclomatic complexity but it should be judged
> > separately in every case.
> +1
> I like it as a suggestion, but sometimes it's just off and better be
> ignored.
>
> > 2) foreman is red
> >
> > This can happen because of intermittent tests failures. If the PR is
> > clearly not causing new ones and commiter is aware of this error, the PR
> > is merged with message like "test unrelated" comments. If we are not
> > sure, we retrigger the run,
> >
> > If Foreman develop branch is broken, we need to merge the PR to fix it so
> > this is another exception. Usually we trigger the jenkins job manually
> > first to see that the PR fixes the issue.
>
> +1
> Yes, don't merge a PR with failing Foreman core tests.
>
> > 3) katello is red
> >
> > If katello becomes red only for this PR, we analyze what causes it.
> > Usually it means that we change some internal things that have impact on
> > Katello. In such case, we're doing our best to send a fixing PR to
> > Katello or we ping someone with better knowledge in this area. We don't
> > merge the PR until it's resolved, then usually we merge both parts at the
> > same time.
>
> I think, this is totally unfair to all the "smaller" plugin maintainers
> and that's why I vote for removing the test completely or just keep it
> to test our public APIs.
> I believe, we should do the following:
> If the Foreman PR breaks some public API, e.g. facets, and the Katello
> tests show that, my suggestion is to fix the foreman PR to not break the
> public API and add proper depreciations if possible.
> If we change something inside Foreman - in the past we changed the host
> multiple actions from GET to POST or introduced strong parameters for
> example - the contributor or maintainer should send a mail to
> foreman-dev expaining what needs to be changed in plugins. I think it's
> also a good idea to fix the example plugin or the How to create a plugin
> wiki page if applicable.
> However I think, it's the plugin maintainer's responsibility to make
> sure his plugin works with Foreman. Everything else doesn't scale. In
> the past a lot of "my" plugins broke because of changes to Foreman core.
> Nobody cared to send a PR so far. But that's fine. I don't expect
> anybody to. It's my job to test the plugin and fix it if it breaks.
> I think, we should not block Foreman PRs because an additional parameter
> was added to some internal method, just because Katello happens to
> overwrite that method. It just doesn't make any sense to me why we
> should do that for Katello but not for all the other plugins out there.
> This is not against Katello, it's just the only plugin tested with
> Foreman core right now.
> Currently, we're developing a plugin to show system logfiles in Foreman.
> That requires a complete ELK-stack for development. I would not expect
> every developer to have that at hand.
> If we leave Katello in the matrix, I think it would be totally fair to
> also add our new plugin to Foreman's test matrix as well. But I wouldn't
> want to block Foreman development just because some test in that plugin
> breaks.
> I know, Katello is important for RedHat and it's one of the larger
> plugins. But that doesn't justify a special role in my opinion.

I understand your feeling. A "justification" for me is that Katello is the
largest plugin we have and therefore is much more prone to be broken by
changes in Foreman. The more code you touch from plugin the higher the chance
is that new core PR breaks something. Also I think for core it's a good way to
find out what impacts our changes have. By testing Katello we get early notice
about something that can impact all plugins. The PR author can consider
whether there's less breaking way of doing the change.

Having said that I still can understand that other plugin maintainers feel
it's unfair. But instead of dropping Katello from matrix, I think the opposite
approach would make more sense. I'd like to see many more plugins tested. I
think plugin test sets are usually much smaller than core one, so it shouldn't
take too much computation time.

In such case I think we'd need a criteria for which plugin should be covered.
Feel free to ask me to start separate thread, but few thoughts what these
could be: the plugin lives under theforeman organization on github, the plugin
is packaged in foreman-packaging, the plugin has support in foreman-installer.
It's funny that Katello does not technically meet any of these but I think
it's being worked on.

Even if such job wouldn't block the core PR to be merged, I as a plugin
maintainer would at least see a warning that my plugin was broken. In ideal
case, the core PR author would consider keeping the change backwards
compatible.

··· On čtvrtek 31. srpna 2017 23:08:34 CEST Timo Goebel wrote: > Am 28.08.17 um 17:12 schrieb Marek Hulán:

–
Marek

If it turns out there are more PRs that are failing with same errors, we
merge PRs if we’re sure they don’t introduce new Katello failures. At
this time, we’re not blocking merges until Katello is green again. (*)
here the suggestion applies

  1. upgrade is red

this is very similar to katello job, if there’s some issue in upgrade, we
should not merge the PR. I remember though, there was a time when we knew
the job is broken which fall under “known to be broken” category.

+1, if upgrade is broken the PR is broken.

  1. There’s no 5, all the rest must be green. Sometimes hound service does
    not respond and remains in “running” state, then it’s retriggered by the
    reviewer. prprocessor and travis must always be happy.

Don’t get me started on hound. But it’s the best we have right now, so
generally speaking: Yes: Linting is important. If there are lint
warnings, we shouldn’t merge the PR.

Now the promised suggestion. When we see katello/upgrade job is broken on
multiple PRs, the first reviewer who spots it should notify the rest of
developers about “from now on, we ignore tests XY”. The ideal channel
seems to be this list. This helps Katello devs to find out when it
started, what were the commits that first induced it.

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

–
Marek

Timo

6

>> Hello devs,
>>
>> since there was a discussion on foreman-dev IRC channel recently about merging
>> PRs in Foreman core even if there's some build failed, I talked to few people
>> and decided to describe here what I think is current way of how it works. I'm
>> also attaching one suggestion at the end that came up after the discussion.
>>
>> Please, add questions, comments or simple +1 so we all know whether we're on
>> the same page.
>>
>> Core PR runs 7 checks - foreman, katello, codeclimate, hound, prprocessor,
>> upgrade, continuous-integration/travis-ci/pr. In ideal case they are all green
>> and after review, the PR is merged. There are several cases where we can merge
>> even if the PR is red.
>>
>> 1) codeclimate is red
>>
>> This can be ignored, we never agreed on using this as a hard metric for the
>> PR. The motivation to introduce it was mainly to save some time to reviewer.
>> We don't have to run it manually to get indications whether there's something
>> introducing a big complexity [1]. From my experience it sometimes leads to
>> worse code, since author splits the logic into more methods to lower e.g.
>> cyclomatic complexity but it should be judged separately in every case.
>
> It should be taken care of whenever possible. Most of the times it's
> certainly right and notices typical problems like stupidly long classes,
> duplication, etc…
>
> https://codeclimate.com/github/rails/rails itself enforces it unless a
> maintainer vets the PR. (ad hominem fallacy, I know)
>
>>
>> 2) foreman is red
>>
>> This can happen because of intermittent tests failures. If the PR is clearly
>> not causing new ones and commiter is aware of this error, the PR is merged
>> with message like "test unrelated" comments. If we are not sure, we retrigger
>> the run,
>>
>> If Foreman develop branch is broken, we need to merge the PR to fix it so this
>> is another exception. Usually we trigger the jenkins job manually first to see
>> that the PR fixes the issue.
>
> Makes sense to me, generally. The only con I've seen since I started
> contributing is that few people care to fix intermittent tests, which
> caused the job to be red for weeks at times

The problem with intermittent issues, or broken builds that are not related
to the PR itself, is that it's not clear whether somebody is already
working on it or not.

For intermittent issues, would it make sense to track every such an
issue in redmine (we perhaps already do)
and add vote for it every time it occurs (not sure if one person can
vote multiple times for some issue).
The goal would be to take this issues as one input of planning and
making sure we put it on the iteration.

For the master failures, having a foreman-dev thread as soon as it
appears and either
inform that one is working on fixing that, or ask for somebody else to
look into it, could work.
Also irc status about that would be useful to know right away what's going on.

>
>>
>> 3) katello is red
>>
>> If katello becomes red only for this PR, we analyze what causes it. Usually it
>> means that we change some internal things that have impact on Katello. In such
>> case, we're doing our best to send a fixing PR to Katello or we ping someone
>> with better knowledge in this area. We don't merge the PR until it's resolved,
>> then usually we merge both parts at the same time.
>
> This sounds good. Ideally the contributor sends the patch to Katello, please.

+1

>>
>> If it turns out there are more PRs that are failing with same errors, we merge
>> PRs if we're sure they don't introduce new Katello failures. At this time,
>> we're not blocking merges until Katello is green again. (*) here the
>> suggestion applies
>
> I don't think this is fair. If the Katello job is red, it's our
> responsibility to bring it back to green. When the causes for the job to
> be red are unknown, merging more stuff in Foreman will certainly NOT
> make it easier to understand them. In fact it may just aggravate the
> problem. So I would say no - at least on PRs I'm reviewing, I'm not
> merging if Katello is red.

If it's clear the PR is not the reason for the failures, there should
not be a reason
for blocking the PR from merging. For the investigation, one needs to
find the first
occurrence of that error in the history, which should be enough to
find exact time
when it started happening.

>
>>
>> 4) upgrade is red
>>
>> this is very similar to katello job, if there's some issue in upgrade, we
>> should not merge the PR. I remember though, there was a time when we knew the
>> job is broken which fall under "known to be broken" category.
>
> Same as 3.

This sound more like broken master case for me.

>
>>
>> 5) There's no 5, all the rest must be green. Sometimes hound service does not
>> respond and remains in "running" state, then it's retriggered by the reviewer.
>> prprocessor and travis must always be happy.
>>
>> Now the promised suggestion. When we see katello/upgrade job is broken on
>> multiple PRs, the first reviewer who spots it should notify the rest of
>> developers about "from now on, we ignore tests XY". The ideal channel seems to
>> be this list. This helps Katello devs to find out when it started, what were
>> the commits that first induced it.
>
> Sorry, I don't think this is a better idea than the current (unspoken)
> approach to block PRs until Katello is merged. It's good you started
> this thread, as clearly people have different assumptions on this topic
> as we didn't talk about it much.

I agree with Marek here, provided it's clear we are in 'katello broken' mode,
what test failures can be ignored for now and that somebody is working
on resolving them.
The last time, it seemed the issue was the Katello team was not even
aware of them
and found it out accidentally (please correct me if that was not the case)

> If we reach to some conclusions, updating
> Foreman :: Contribute with them would be good to have
> some reference set in stone (or in bytes (: ).

+1 on documenting the result of this discussion there.

··· On Tue, Aug 29, 2017 at 11:25 AM, Daniel Lobato Garcia wrote: > On 08/28, Marek Hulán wrote: > >> >> [1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk >> >> Thanks for comments >> >> -- >> Marek >> >> -- >> You received this message because you are subscribed to the Google Groups "foreman-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. > > -- > Daniel Lobato Garcia > > @dLobatog > blog.daniellobato.me > daniellobato.me > > GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30 > Keybase: https://keybase.io/elobato > > -- > You received this message because you are subscribed to the Google Groups "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
7

> Thanks Timo for your input. Please see my comment below in the text.
>
>> > 1) codeclimate is red
>> >
>> > This can be ignored, we never agreed on using this as a hard metric for
>> > the
>> > PR. The motivation to introduce it was mainly to save some time to
>> > reviewer. We don't have to run it manually to get indications whether
>> > there's something introducing a big complexity [1]. From my experience it
>> > sometimes leads to worse code, since author splits the logic into more
>> > methods to lower e.g. cyclomatic complexity but it should be judged
>> > separately in every case.
>> +1
>> I like it as a suggestion, but sometimes it's just off and better be
>> ignored.
>>
>> > 2) foreman is red
>> >
>> > This can happen because of intermittent tests failures. If the PR is
>> > clearly not causing new ones and commiter is aware of this error, the PR
>> > is merged with message like "test unrelated" comments. If we are not
>> > sure, we retrigger the run,
>> >
>> > If Foreman develop branch is broken, we need to merge the PR to fix it so
>> > this is another exception. Usually we trigger the jenkins job manually
>> > first to see that the PR fixes the issue.
>>
>> +1
>> Yes, don't merge a PR with failing Foreman core tests.
>>
>> > 3) katello is red
>> >
>> > If katello becomes red only for this PR, we analyze what causes it.
>> > Usually it means that we change some internal things that have impact on
>> > Katello. In such case, we're doing our best to send a fixing PR to
>> > Katello or we ping someone with better knowledge in this area. We don't
>> > merge the PR until it's resolved, then usually we merge both parts at the
>> > same time.
>>
>> I think, this is totally unfair to all the "smaller" plugin maintainers
>> and that's why I vote for removing the test completely or just keep it
>> to test our public APIs.
>> I believe, we should do the following:
>> If the Foreman PR breaks some public API, e.g. facets, and the Katello
>> tests show that, my suggestion is to fix the foreman PR to not break the
>> public API and add proper depreciations if possible.
>> If we change something inside Foreman - in the past we changed the host
>> multiple actions from GET to POST or introduced strong parameters for
>> example - the contributor or maintainer should send a mail to
>> foreman-dev expaining what needs to be changed in plugins. I think it's
>> also a good idea to fix the example plugin or the How to create a plugin
>> wiki page if applicable.
>> However I think, it's the plugin maintainer's responsibility to make
>> sure his plugin works with Foreman. Everything else doesn't scale. In
>> the past a lot of "my" plugins broke because of changes to Foreman core.
>> Nobody cared to send a PR so far. But that's fine. I don't expect
>> anybody to. It's my job to test the plugin and fix it if it breaks.
>> I think, we should not block Foreman PRs because an additional parameter
>> was added to some internal method, just because Katello happens to
>> overwrite that method. It just doesn't make any sense to me why we
>> should do that for Katello but not for all the other plugins out there.
>> This is not against Katello, it's just the only plugin tested with
>> Foreman core right now.
>> Currently, we're developing a plugin to show system logfiles in Foreman.
>> That requires a complete ELK-stack for development. I would not expect
>> every developer to have that at hand.
>> If we leave Katello in the matrix, I think it would be totally fair to
>> also add our new plugin to Foreman's test matrix as well. But I wouldn't
>> want to block Foreman development just because some test in that plugin
>> breaks.
>> I know, Katello is important for RedHat and it's one of the larger
>> plugins. But that doesn't justify a special role in my opinion.
>
> I understand your feeling. A "justification" for me is that Katello is the
> largest plugin we have and therefore is much more prone to be broken by
> changes in Foreman. The more code you touch from plugin the higher the chance
> is that new core PR breaks something. Also I think for core it's a good way to
> find out what impacts our changes have. By testing Katello we get early notice
> about something that can impact all plugins. The PR author can consider
> whether there's less breaking way of doing the change.
>
> Having said that I still can understand that other plugin maintainers feel
> it's unfair. But instead of dropping Katello from matrix, I think the opposite
> approach would make more sense. I'd like to see many more plugins tested. I
> think plugin test sets are usually much smaller than core one, so it shouldn't
> take too much computation time.

+1 - we should probably skip running the foreman tests again with the plugin
in this matrix, as this is usually the longest time in the test run for plugins.

>
> In such case I think we'd need a criteria for which plugin should be covered.
> Feel free to ask me to start separate thread, but few thoughts what these
> could be: the plugin lives under theforeman organization on github, the plugin
> is packaged in foreman-packaging, the plugin has support in foreman-installer.
> It's funny that Katello does not technically meet any of these but I think
> it's being worked on.
>
> Even if such job wouldn't block the core PR to be merged, I as a plugin
> maintainer would at least see a warning that my plugin was broken. In ideal
> case, the core PR author would consider keeping the change backwards
> compatible.

X most used plugins could be a good criteria, which would give us some insights
on how the changes in core influence the wider ecosystem.

– Ivan

··· On Fri, Sep 1, 2017 at 8:56 AM, Marek Hulán wrote: > On čtvrtek 31. srpna 2017 23:08:34 CEST Timo Goebel wrote: >> Am 28.08.17 um 17:12 schrieb Marek Hulán:

–
Marek

If it turns out there are more PRs that are failing with same errors, we
merge PRs if we’re sure they don’t introduce new Katello failures. At
this time, we’re not blocking merges until Katello is green again. (*)
here the suggestion applies

  1. upgrade is red

this is very similar to katello job, if there’s some issue in upgrade, we
should not merge the PR. I remember though, there was a time when we knew
the job is broken which fall under “known to be broken” category.

+1, if upgrade is broken the PR is broken.

  1. There’s no 5, all the rest must be green. Sometimes hound service does
    not respond and remains in “running” state, then it’s retriggered by the
    reviewer. prprocessor and travis must always be happy.

Don’t get me started on hound. But it’s the best we have right now, so
generally speaking: Yes: Linting is important. If there are lint
warnings, we shouldn’t merge the PR.

Now the promised suggestion. When we see katello/upgrade job is broken on
multiple PRs, the first reviewer who spots it should notify the rest of
developers about “from now on, we ignore tests XY”. The ideal channel
seems to be this list. This helps Katello devs to find out when it
started, what were the commits that first induced it.

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

–
Marek

Timo

–
You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

8

Hello,

In general, I agree with what Marek said. Since I was part of the original
discussion that started this whole thread, I feel obliged to put in my 2¢.

Obviously we all want test to be all green before merging any PR. Still,
there are certain cases where tests are red in which I believe using our
discretion is enough to merge nonetheless:

  1. Intermittent failures in core - sometimes these issues are very
    difficult to hunt down, and in other cases are caused by things like
    network outages which are almost impossible to completely prevent. These
    are very easy to notice since they are usually red in just 1 combination in
    the matrix with the others green, and are almost always related to ui js
    timeouts. Rerunning the tests in these cases would most likely make them
    green again (or lead to another intermittent failure). However, re-running
    tests in this case causes another job to be added to the already long
    jenkins queue, and requires the reviewer to remember to check if it passed
    an hour or two later.
  2. Code Climate - I agree, this should only be taken as a guideline, not as
    a requirement.
  3. Failures in plugins (currently we only test Katello but I think we
    should test more - we already discussed this) - Here there are 3 distinct
    options:
    a. Intermittent failure in plugin - this can be ignored imho only for
    extremely minor changes - e.g. string changed, minor ui fix etc. meaning -
    only changes that definitely don't touch in an area that should break
    plugins. If in doubt- rerun the test.
    b. Plugin is broken but not due to the PR - if multiple PRs are failing
    with the same errors and they aren't related to the change in the PR, most
    chances are that the plugin is broken due to changes implemented in it or
    in some other dependency. In this case I believe it doesn't make sense to
    block merging in core - it may take several days in some cases for such
    issues to be resolved, and stopping all work on core until that happens
    doesn't make much sense. Precaution should be taken not to merge PRs that
    cause further issues for the plugin, but if failures are limited to the
    same tests I believe that is enough indication to allow merge.
    c. PR breaks plugin - Ideally, this shouldn't ever happen - if you are
    going to break something, deprecate first and give plugins time to adjust.
    Sometimes this can't be done because reasons, in which case every effort
    should be taken to make sure plugins are changed accordingly before merging
    (either by opening PRs or asking assistance from relevant devs).
  4. any other failure - rerun tests if not sure if it's related or not.

In any case, informing the list of any failures is a good idea, since
otherwise they may go to redmine to die (or are completely ignored) with
no-one noticing them, or on the other hand - needlessly preventing merges
due to them.

Perhaps I'm a bit lenient in my approach, but I'd rather merge changes if
I'm feeling certain enough that the change doesn't break anything and risk
being wrong once in a blue moon (I don't think I recall ever merging with a
red test and breaking something in the almost 2 years I've been a core
maintainer). Our review process is already very slow, let's not add more
difficulty to it.

Tomer

··· On Fri, Sep 1, 2017 at 10:36 AM, Ivan Necas wrote:

On Fri, Sep 1, 2017 at 8:56 AM, Marek Hulán mhulan@redhat.com wrote:

Thanks Timo for your input. Please see my comment below in the text.

On čtvrtek 31. srpna 2017 23:08:34 CEST Timo Goebel wrote:

Am 28.08.17 um 17:12 schrieb Marek Hulán:

  1. codeclimate is red

This can be ignored, we never agreed on using this as a hard metric
for

the
PR. The motivation to introduce it was mainly to save some time to
reviewer. We don’t have to run it manually to get indications whether
there’s something introducing a big complexity [1]. From my
experience it

sometimes leads to worse code, since author splits the logic into more
methods to lower e.g. cyclomatic complexity but it should be judged
separately in every case.
+1
I like it as a suggestion, but sometimes it’s just off and better be
ignored.

  1. foreman is red

This can happen because of intermittent tests failures. If the PR is
clearly not causing new ones and commiter is aware of this error, the
PR

is merged with message like “test unrelated” comments. If we are not
sure, we retrigger the run,

If Foreman develop branch is broken, we need to merge the PR to fix
it so

this is another exception. Usually we trigger the jenkins job manually
first to see that the PR fixes the issue.

+1
Yes, don’t merge a PR with failing Foreman core tests.

  1. katello is red

If katello becomes red only for this PR, we analyze what causes it.
Usually it means that we change some internal things that have impact
on

Katello. In such case, we’re doing our best to send a fixing PR to
Katello or we ping someone with better knowledge in this area. We
don’t

merge the PR until it’s resolved, then usually we merge both parts at
the

same time.

I think, this is totally unfair to all the “smaller” plugin maintainers
and that’s why I vote for removing the test completely or just keep it
to test our public APIs.
I believe, we should do the following:
If the Foreman PR breaks some public API, e.g. facets, and the Katello
tests show that, my suggestion is to fix the foreman PR to not break the
public API and add proper depreciations if possible.
If we change something inside Foreman - in the past we changed the host
multiple actions from GET to POST or introduced strong parameters for
example - the contributor or maintainer should send a mail to
foreman-dev expaining what needs to be changed in plugins. I think it’s
also a good idea to fix the example plugin or the How to create a plugin
wiki page if applicable.
However I think, it’s the plugin maintainer’s responsibility to make
sure his plugin works with Foreman. Everything else doesn’t scale. In
the past a lot of “my” plugins broke because of changes to Foreman core.
Nobody cared to send a PR so far. But that’s fine. I don’t expect
anybody to. It’s my job to test the plugin and fix it if it breaks.
I think, we should not block Foreman PRs because an additional parameter
was added to some internal method, just because Katello happens to
overwrite that method. It just doesn’t make any sense to me why we
should do that for Katello but not for all the other plugins out there.
This is not against Katello, it’s just the only plugin tested with
Foreman core right now.
Currently, we’re developing a plugin to show system logfiles in Foreman.
That requires a complete ELK-stack for development. I would not expect
every developer to have that at hand.
If we leave Katello in the matrix, I think it would be totally fair to
also add our new plugin to Foreman’s test matrix as well. But I wouldn’t
want to block Foreman development just because some test in that plugin
breaks.
I know, Katello is important for RedHat and it’s one of the larger
plugins. But that doesn’t justify a special role in my opinion.

I understand your feeling. A “justification” for me is that Katello is
the
largest plugin we have and therefore is much more prone to be broken by
changes in Foreman. The more code you touch from plugin the higher the
chance
is that new core PR breaks something. Also I think for core it’s a good
way to
find out what impacts our changes have. By testing Katello we get early
notice
about something that can impact all plugins. The PR author can consider
whether there’s less breaking way of doing the change.

Having said that I still can understand that other plugin maintainers
feel
it’s unfair. But instead of dropping Katello from matrix, I think the
opposite
approach would make more sense. I’d like to see many more plugins
tested. I
think plugin test sets are usually much smaller than core one, so it
shouldn’t
take too much computation time.

+1 - we should probably skip running the foreman tests again with the
plugin
in this matrix, as this is usually the longest time in the test run for
plugins.

In such case I think we’d need a criteria for which plugin should be
covered.
Feel free to ask me to start separate thread, but few thoughts what these
could be: the plugin lives under theforeman organization on github, the
plugin
is packaged in foreman-packaging, the plugin has support in
foreman-installer.
It’s funny that Katello does not technically meet any of these but I
think
it’s being worked on.

Even if such job wouldn’t block the core PR to be merged, I as a plugin
maintainer would at least see a warning that my plugin was broken. In
ideal
case, the core PR author would consider keeping the change backwards
compatible.

X most used plugins could be a good criteria, which would give us some
insights
on how the changes in core influence the wider ecosystem.

– Ivan

–
Marek

If it turns out there are more PRs that are failing with same errors,
we

merge PRs if we’re sure they don’t introduce new Katello failures. At
this time, we’re not blocking merges until Katello is green again. (*)
here the suggestion applies

  1. upgrade is red

this is very similar to katello job, if there’s some issue in
upgrade, we

should not merge the PR. I remember though, there was a time when we
knew

the job is broken which fall under “known to be broken” category.

+1, if upgrade is broken the PR is broken.

  1. There’s no 5, all the rest must be green. Sometimes hound service
    does

not respond and remains in “running” state, then it’s retriggered by
the

reviewer. prprocessor and travis must always be happy.

Don’t get me started on hound. But it’s the best we have right now, so
generally speaking: Yes: Linting is important. If there are lint
warnings, we shouldn’t merge the PR.

Now the promised suggestion. When we see katello/upgrade job is
broken on

multiple PRs, the first reviewer who spots it should notify the rest
of

developers about “from now on, we ignore tests XY”. The ideal channel
seems to be this list. This helps Katello devs to find out when it
started, what were the commits that first induced it.

[1] https://groups.google.com/forum/#!topic/foreman-dev/p7ESagXwNwk

Thanks for comments

–
Marek

Timo

–
You received this message because you are subscribed to the Google
Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send
an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
Have a nice day,
Tomer Brisker
Red Hat Engineering