support non critical triggered schedulers

[Riziero]

This is not done yet. Some tests are failing. Just wanted to know if my approach is acceptable.

[mention-bot]

By analyzing the blame information on this pull request, we identified @tardyp, @warner and @djmitche to be potential reviewers

[mention-bot]

By analyzing the blame information on this pull request, we identified @tardyp, @warner and @djmitche to be potential reviewers

[tardyp]

Can you explain the details of you needs, and your design?

[tardyp]

I think the best is to start with the documentation. This helps to design a proper api change that is clear and usable by everybody.

For what I undestand your usecase looks interresting to have

[Riziero]

@tardyp

Right now when you have a trigger step kicking a number od schedullers, any fail of the schedulers will cause the Trigger step to be marked as failed. In my use case, while I still want to wait for all the kicked schedulers to finish, I want to be able to mark a scheduler as "non critical". When such a scheduler fails, the kicking trigget step will still be green.

With my code you can still do

factory.addStep(steps.Trigger(schedulerNames=['triggerable_2_fail', 'triggerable_1_green'], waitForFinish=False))

but you can also do:

factory.addStep(steps.Trigger(schedulerNames=[('triggerable_1_green', True), ('triggerable_2_fail', False)], waitForFinish=True))

which reads: if triggerable_2_fail fails the trigger step is "still green"

[tardyp]

Here what worries me the most is that your new api is not compatible with the old API.
This is very important for us that our API moves always in a backward compatible way.

Also, for me implicit meaning for a tuple looks not very easy to understand. You change the meaning of schedulerNames to be no more a list of scheduler names, and be a list of schedulername + whether to take the result in account for the global result.

I think I would prefer to have an additional unimportantSchedulerNames argument that worstStatus will take in account

[Riziero]

Cool, that was my concern too. I'll fix it up and upload again! Thanks

[codecov-io]

Current coverage is 85.83%

Merging #2299 into master will not change coverage

@@             master      #2299   diff @@
==========================================
  Files           293        293          
  Lines         30528      30528          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          26205      26205          
  Misses         4323       4323          
  Partials          0          0          

Powered by Codecov. Last updated by e925c49...459b656

[tardyp]

You can reset your branch to buildbot/master no need to revert the last commit.

[Riziero]

would it be fine to change the return value of getSchedulersAndProperties()?

it's now returning a list. I would have it return the same list plus "unimportantSchedulerNames". This way users can also choose unimportantSchedulerNames in a dynamic trigger

[tardyp]

I think it would make sense, but we need to make it backward compatible

[tardyp]

I'm closing since it does not have code anymore. Please reopen when you find the time to implement the agreed solution