packit-service icon indicating copy to clipboard operation
packit-service copied to clipboard

Don't add reaction on comment when the command will be ignored

Open nikromen opened this issue 3 years ago • 8 comments

We give :eyes: reaction to comments that are later ignored. This may confuse user. And we do that in several situations.

  • when stg is not in packit_instances and /packit-stg command is triggered. The same for prod instance
  • when some job is triggered (/packit build, /packit propose-downstream, etc) but the corresponding job is not located in .packit.yaml file

You can try this behavior by commenting to this PR with /packit-stg prefix. Or to this PR by commenting with wrong job - /packit test.

This is caused because reaction to comment is given when we are processing Events but packit_instances and matching handlers are checked after that.

Way to solve this: give reaction later (after checking packit_instances and matching handlers) so we give :eyes: reaction only when it will be followed by some other action (after all this is purpose of our bot giving a reaction)

so the goal is move part where we give a reaction in jobs.py

event.comment_object.add_reaction(COMMENT_REACTION)

somewhere "further" in code - after we check for matching handlers, job configs and after checking packit_instances

nikromen avatar Apr 13 '22 11:04 nikromen

This issue has been marked as stale because it hasn't seen any activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! :unicorn: :rocket: :robot:

(Note: issues labeled with pinned or EPIC are never marked as stale.)

stale[bot] avatar Jul 10 '22 13:07 stale[bot]

This is still valid, see https://github.com/packit/hello-world/pull/494#issuecomment-1339253487

lbarcziova avatar Dec 06 '22 12:12 lbarcziova

I think I don't understand this issue. When I check your comment, Laura, Packit's actions are as expected to me:

You asked stg to process the job, there is only one set of eyes: image

And the eyes are applied by the stg app image

prod app did not interact with the comment in any way

all seems correct to me

TomasTomecek avatar Dec 06 '22 13:12 TomasTomecek

The problem is staging should not react to that PR (packit_instances is not specified in the config => by default only prod app should react) and it correctly did not trigger the builds, but still reacted with eyes.

lbarcziova avatar Dec 06 '22 13:12 lbarcziova

ahaaaaa, so it's about packit_instances, did not realize that; thank you for the explanation

TomasTomecek avatar Dec 06 '22 15:12 TomasTomecek

Hi! Would this also solve a situation, when the test job is requested whilst builds as dependencies are not finished? The bot reacts to the comment/command (/packit test), but the test jobs won't get triggered, when build tasks finish. For context all the test jobs are defined as manual in the config file, build jobs are set for the commit/push trigger.

danmyway avatar Jan 29 '24 15:01 danmyway

As an alternative, would it work to add a reaction ("I saw it") and then an error message as a reply to the comment explaining why the command is ignored? The situations where this happens look to me like errors that should be reported somehow and not silently ignored.

pcahyna avatar Jun 03 '24 14:06 pcahyna

An error message would be more useful than not adding a reaction, which would be a bit less confusing, but still would not really help the user with debugging. The user would be still left wondering why the reaction does not appear, like now they are wondering why nothing happens. One really needs explanatory error messages, not just an emoji. (Reminds me of the only way ed the standard text editor reported errors: a lone ?. https://news.ycombinator.com/item?id=7727206 )

pcahyna avatar Jun 03 '24 14:06 pcahyna