Implement a label_trigger option for test jobs

[psss]

Description

Similar to manual_trigger the label_trigger would enable specific test job only if given label is attached to the pull request.

Benefit

Once the pull request has passed basic sanity tests or an agreed level of code review, it would be marked for example with a full test label and all future pull request changes would be tested without need to again and again manually add /packit test --identifier full comments. Also this would resolve the problem with having to wait until the copr builds are ready, see #2265 for details.

Importance

This would substantially improve the several-tier-testing experience. And prevent unnecessary manual steps.

What is the impacted category (job)?

Testing Farm tests

Workaround

• There is an existing workaround that can be used until this feature is implemented.

Participation

• I am willing to submit a pull request for this issue. (Packit team is happy to help!)

[psss]

As @lachmanfrantisek mentioned on the chat, this should behave like a condition (during the job scheduling the label would be just checked for presence) rather than a trigger (like watching the event when the label was added), so maybe the proposed option name is a bit misleading.

[psss]

Is there any ETA on implementing this?

[lbarcziova]

hi @psss, I have put this to ready-to-refine, so I hope we could get to this in the next 2-3 weeks.

[psss]

Great, thanks for the update!

[majamassarini]

If possible also check for labels to not be present. Think about one implementation that can be reused in more jobs.

[mfocko]

With regards to the previous comment, I would suggest something like:

label_trigger:
  present:
    - test_regressions
  absent:
    - skip_tests
    - ci_skip

To clarify what we discussed during the refinement, we also considered being able to run jobs only if there are labels missing, the example above probably explains it well.

[psss]

Negative filter sounds useful. The proposed syntax looks good. Perhaps, one more brainstorming idea: What about this?

trigger:
    label:
        present:
          - test_regressions
        absent:
          - skip_tests
          - ci_skip

[mfocko]

Did you have something like

trigger:
    label:
        present:
          - test_regressions
        absent:
          - skip_tests
          - ci_skip
    manual: true

in mind? With manual_trigger as backwards-compatible and to-be-deprecated it sounds pretty reasonable 🤔

[psss]

Yeah, that was the idea, to group all triggers under a single trigger umbrella.

[lbarcziova]

As @lachmanfrantisek mentioned on the chat, this should behave like a condition (during the job scheduling the label would be just checked for presence) rather than a trigger (like watching the event when the label was added), so maybe the proposed option name is a bit misleading.

Getting back to this, I would probably omit the trigger word from these options and go with

labels:
  present:
    - test_regressions
  absent:
    - skip_tests

or maybe

label_conditions:
  present:
    - test_regressions
  absent:
    - skip_tests

WDYT?

[psss]

rather than a trigger (like watching the event when the label was added)

Maybe it would be good to clarify the expected behavior first: What will happen if a new label is added to the pull request and there are jobs (marked with that label) already waiting in the queue? I'd say it would be nice if the missing jobs are started right away.

[lbarcziova]

From your comment and also checking the thread on Slack, I think we agreed it would behave only like a condition, so adding/removing a label would not trigger Packit's action.

[psss]

Ok, so it should behave only as a condition. Do we expect there will/could be other conditions? If so, the following would be a bit more extensible syntax:

condition:
    label:
        present:
          - test_regressions
        absent:
          - skip_tests
          - ci_skip

Single key labels would collide with packit job labels, or not?

[lbarcziova]

Single key labels would collide with packit job labels, or not?

oh yes, that's true

I don't recall any other conditions discussed so far, but I am still for the idea to have if extensible, so your suggestion sounds good to me.

[psss]

Thanks for implementing this! Trying in:

• Enable the full test execution using a label teemtee/tmt#2683

Seems that the internal jobs have been scheduled. I guess the change has not been deployed yet?

[mfocko]

That would fail on parsing the config 👀

[psss]

I see. Then, it seems, it does not work as expected.

[lbarcziova]

@psss yes, there is still one piece for build- > test mapping missing, I hope I will get it to prod until tomorrow, I will let you know :) (at the moment, you would need to add the require.label also for the build job to make it work)

[psss]

Understood. Thanks for the quick response! The build workaround is probably not an option because we still need the build to be completed for the basic sanity jobs which are enabled by default.

By the way, the manual_trigger is not needed when the require key is used, right?

[lbarcziova]

By the way, the manual_trigger is not needed when the require key is used, right?

Hmm not sure I follow. If there is the require.label and the labels will not match, nothing will happen. On the other hand, if the PR will have the needed labels, but there will be no manual_trigger: True, also the pushes to PR will trigger tests.