[OCPERT-163] Send drop-bug MR notification to ART team on slack #698

jhuttana · 2025-10-03T05:46:21Z

At present ART is requested manually to take a look at drop-bugs MR. This PR aims to automate the same.

openshift-ci · 2025-10-03T05:46:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tomasdavidorg for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LuboTerifaj

Hello @jhuttana ,

please have a look at my comments.

Thanks!

LuboTerifaj · 2025-10-03T14:14:42Z

oar/cli/cmd_drop_bugs.py

            nm.share_dropped_bugs(dropped_bugs)
+            # Notify ART using MR url directly from ShipmentData cache (no extra GitLab lookup)
+            try:
+                from oar.core.shipment import ShipmentData


I think it would be better to move the import to the imports section at the beginning of the file.

LuboTerifaj · 2025-10-03T14:21:43Z

oar/core/shipment.py

+            # cache existing MR url for notification
+            try:
+                self._last_drop_bugs_mr_url = mr.get_web_url()
+            except Exception:


We should handle the exception and log the error, and not just ignore it.

LuboTerifaj · 2025-10-03T14:22:59Z

oar/core/shipment.py

+            # cache new MR url for notification
+            try:
+                self._last_drop_bugs_mr_url = new_mr.get_web_url()
+            except Exception:


We should handle the exception and log the error, and not just ignore it.

LuboTerifaj · 2025-10-03T14:27:44Z

oar/core/shipment.py

            gh.configure_remotes("origin", f"https://group_143087_bot_e4ed5153eb7e7dfa7eb3d7901a95a6a7:{self._cs.get_gitlab_token()}@gitlab.cee.redhat.com/rioliu/ocp-shipment-data.git")
+            # cache existing MR url for notification
+            try:
+                self._last_drop_bugs_mr_url = mr.get_web_url()


Why are you setting the shipment merge request as a drop bugs mr?

Good catch! I will make the suitable changes

LuboTerifaj · 2025-10-03T14:43:53Z

oar/core/shipment.py

        return self._mr

+    def get_last_drop_bugs_mr_url(self) -> str | None:
+        """Return the last drop-bugs MR URL detected/created during drop_bugs()."""


I would suggest to use the same docstring style as used in other methods in this file.

In addition, I think the current description may be a little bit confusing. I would change it to clarify what the "last" means, and what is the "drop_bugs()" to something like: Returns the URL of the latest drop-bugs Merge Request (MR) that was either detected or created during the drop_bugs() execution.

Will correct it in the next commit

LuboTerifaj · 2025-10-03T14:59:19Z

oar/core/notification.py

+            self._mh = MessageHelper(self.cs)
+        return self._mh
+
+    def _get_channel(self, contact_key: str) -> str:


docstring would be useful for this method.

What is your motivation for caching? Did you stumble on any issues without it?

Optimized performance by reducing repeated lookups on hot paths and keeping future costs bounded. The _get_channel method eventually invokes ConfigStore.get_slack_channel_from_contact(...), which, although lightweight today, is called multiple times across various notification methods within a single command. By introducing caching, each contact key is now resolved only once per NotificationManager instance, improving efficiency and reducing redundant lookups.
There is zero behavioral change for the existing notifications; just avoids repeated calls.
Happy to remove it if you want to keep the class minimal, or switch to “compute-once” fields for the 2–3 known keys we use.

I don’t think caching is necessary here, since the values are already loaded into memory when calling

release-tests/oar/core/configstore.py

Line 43 in 4cd7f3a

self._local_conf = json.loads(jwe.decrypt(f.read(), jwk))

.
If you would like to add caching, it should be implemented in the ConfigStore class to preserve proper encapsulation.

LuboTerifaj · 2025-10-03T15:06:42Z

oar/core/notification.py

+        ch = self.cs.get_slack_channel_from_contact(contact_key)
+        self._channel_cache[contact_key] = ch
+        return ch


Suggested change

ch = self.cs.get_slack_channel_from_contact(contact_key)

self._channel_cache[contact_key] = ch

return ch

channel = self.cs.get_slack_channel_from_contact(contact_key)

self._channel_cache[contact_key] = channel

return channel

LuboTerifaj · 2025-10-03T15:28:00Z

oar/cli/cmd_drop_bugs.py

+                sd = ShipmentData(cs)
+                mr_url = sd.get_last_drop_bugs_mr_url()
+                if mr_url:
+                    nm.share_drop_bugs_mr_for_approval(mr_url, dropped_count=len(dropped_bugs))


dropped_count is never used

Suggested change

nm.share_drop_bugs_mr_for_approval(mr_url, dropped_count=len(dropped_bugs))

nm.share_drop_bugs_mr_for_approval(mr_url, len(dropped_bugs))

It is used in notification.py in the slack message.

I meant that you can omit dropped_count and keep just len(dropped_bugs) as positional argument.
This is probably more about your preference and code style.

LuboTerifaj · 2025-10-03T15:33:29Z

oar/core/notification.py

        except Exception as e:
            raise NotificationException("share shipment MR and AD info failed") from e

+    def share_drop_bugs_mr_for_approval(self, mr_url: str, dropped_count: int = 0):


Is there any reason why dropped_count is optional? Do we want to send two types of notifications?

We’re not aiming for two different notifications; it’s the same message with an optional “(dropped N bugs)” suffix. It is just to enrich the message.

If we always want to include information about the number of bugs being dropped, consider making this parameter non-optional.

LuboTerifaj · 2025-10-03T15:50:49Z

tests/test_notification.py

+    def test_lazy_init_and_channel_cache(self):
+        """Validate Slack client lazy init and channel caching reduce repeated lookups"""
+        # Prepare NotificationManager with a fake MessageHelper to avoid external calls
+        class _FakeMH:


Please consider to use Mocks also here instead of substitute class.

rioliu-rh · 2025-10-14T06:50:45Z

/hold

Let's hold this PR, if find-bugs cli changes from ART works well, cmd drop-bugs will be eliminated.

jhuttana · 2025-10-14T06:53:18Z

/hold

Let's hold this PR, if find-bugs cli changes from ART works well, cmd drop-bugs will be eliminated.

@rioliu-rh Sure

openshift-ci · 2025-10-14T07:03:24Z

@jhuttana: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

jhuttana requested a review from rioliu-rh October 3, 2025 05:46

openshift-ci bot requested a review from LuboTerifaj October 3, 2025 05:46

jhuttana requested a review from tomasdavidorg October 3, 2025 05:46

LuboTerifaj requested changes Oct 3, 2025

View reviewed changes

openshift-ci bot assigned LuboTerifaj Oct 3, 2025

jhuttana added 2 commits October 14, 2025 12:17

Send drop-bug MR notification to ART team on slack

9318a66

Address review comments

bac158f

jhuttana force-pushed the drop-bugs-mr-notify branch from 516b65a to bac158f Compare October 14, 2025 06:48

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2025

	nm.share_drop_bugs_mr_for_approval(mr_url, dropped_count=len(dropped_bugs))
	nm.share_drop_bugs_mr_for_approval(mr_url, len(dropped_bugs))

[OCPERT-163] Send drop-bug MR notification to ART team on slack #698

Are you sure you want to change the base?

[OCPERT-163] Send drop-bug MR notification to ART team on slack #698

Uh oh!

Conversation

jhuttana commented Oct 3, 2025

Uh oh!

openshift-ci bot commented Oct 3, 2025

Uh oh!

LuboTerifaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rioliu-rh commented Oct 14, 2025

Uh oh!

jhuttana commented Oct 14, 2025

Uh oh!

openshift-ci bot commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants