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 <ine...@redhat.com> wrote: > On Fri, Sep 1, 2017 at 8:56 AM, Marek Hulán <mhu...@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. > >> > >> > 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 > > > > > -- > > 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 > >> > > >> > 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 > > > > > > -- > > 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+unsubscr...@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+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- Have a nice day, Tomer Brisker Red Hat Engineering -- 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+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
