Packit-service: Remove the "Congratulations!" comment

[TomasTomecek]

Current TODO:

• [ ] add the instructions to the result page
• [ ] add a note about removal to the "Congratulations!" comment
• [ ] remove the commenting after two weeks

An old description follows:

@ernestask reached out to us saying that lately p-s stopped putting the "Congratulations!" comment in their PRs even though they didn't change anything on their side:

https://github.com/abrt/faf/pull/903
https://github.com/abrt/faf/pull/896
https://github.com/abrt/retrace-server/pull/315

In our codebase, we're probably talking about this:

        if (
            self.build_job_helper.job_build
            and self.build_job_helper.job_build.trigger
            == JobConfigTriggerType.pull_request
            and not self.was_last_build_successful()
            and self.package_config.notifications.pull_request.successful_build
        ):
            msg = (
                f"Congratulations! One of the builds has completed. :champagne:\n\n"
                "You can install the built RPMs by following these steps:\n\n"

Let's analyze:

1. self.build_job_helper.job_build they have this defined, and even if they didn't, default is build+test
2. self.build_job_helper.job_build.trigger == JobConfigTriggerType.pull_request 'duh
3. not self.was_last_build_successful() there is no p-s comment, so we're good here
4. self.package_config.notifications.pull_request.successful_build defaults to True

am I missing something here?

[lachmanfrantisek]

1. self.build_job_helper.job_build they have this defined, and even if they didn't, default is build+test

No, only test is the default.

[lachmanfrantisek]

OK, looks like the problem with build x copr-build aliasing.

[csomh]

During grooming we discussed, that we might actually consider dropping the "Congratulations" comments (as currently has no user value—at least this is how @csomh and @TomasTomecek felt), and make sure to have the installation instructions at the top of the build result pages.

This way we could get rid of this code, and just not fix it :smiley:

What do you think @lachmanfrantisek?

[ernestask]

Since I had originally assumed that those were dropped altogether, I am completely fine with such resolution.

[lachmanfrantisek]

Since it's configurable, what about switching the default and see if the people really want it?

[csomh]

Since it's configurable, what about switching the default and see if the people really want it?

Yes, we could start with this, and wait a little to see the reaction. Good idea!

[stale[bot]]

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.)

[lachmanfrantisek]

We discussed this today and agreed that we will:

• [ ] add the instructions to the result page
• [ ] add a note about removal to the "Congratulations!" comment
• [ ] remove the commenting after two weeks

[lachmanfrantisek]

I've updated the description and title.

[lachmanfrantisek]

Sorry for the noise, closing in favour of #881 (deprecation) and #882 (removal) to match the downstream planning.