-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RHCLOUD-22813] Handle provided context.host_url in integration templates #3221
base: master
Are you sure you want to change the base?
Conversation
8f61aa6
to
905a9e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work Jessica :)
Because tis PR will involve some braking changes, I think it should be split in two PRs for camel processor case, like:
- Introduce new entries into data source object
- Update Qute templates and remove deprecated data from data source object (like environnment_url)
WDYT?
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
...se/src/main/resources/db/migration/V1.109.0__RHCLOUD-22813_update_template_host_app_urls.sql
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/InsightsUrlsBuilder.java
Outdated
Show resolved
Hide resolved
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
engine/src/main/java/com/redhat/cloud/notifications/processors/camel/CamelProcessor.java
Show resolved
Hide resolved
Agree with splitting the PRs. I'll open a new one later today. |
e8efc03
to
844fc7d
Compare
CamelProcessor tests and db migration not completed yet
844fc7d
to
7c24129
Compare
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
...y/src/main/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTransformer.java
Show resolved
Hide resolved
...uty/src/test/java/com/redhat/cloud/notifications/connector/pagerduty/PagerDutyTestUtils.java
Outdated
Show resolved
Hide resolved
/retest |
@jessicarod7 it looks like those changes breaks test_integration_pagerduty_event_triggered IQE test. |
wasn't expecting this one to fail, looking into it |
if (!displayName.isEmpty()) { | ||
if (data.getString("bundle", "").equals("openshift") | ||
&& data.getString("application", "").equals("advisor")) { | ||
path = String.format("/openshift/insights/advisor/clusters/%s", displayName); | ||
} else { | ||
path = "/insights/inventory/"; | ||
if (!inventoryId.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using .isBlank()
instead of .isEmpty()
, in case we get sent a " "
string by mistake.
Jira issue
https://issues.redhat.com/browse/RHCLOUD-22813
Description
Currently, URLs in PagerDuty and Camel notifications are constructed by the PagerDutyTransformer, or one of three Qute templates, respectively. Each one has a slightly different way of handling URLs, and the Camel templates are not the ones currently shown in this repo.
This PR creates a new class,
InsightsUrlsBuilder
, which provides consistent construction of the two URLs for all integrations:data.inventory_url
represents a link to the specific host, cluster, or inventory item in the Console that generated the message. Ifdata.context.host_url
is provided, it will be used here.data.application_url
links back to the application that generated the event.The PR will also make it easy to modify the URL format in a followup issue.
Database migration
A new migration file (V1.110.0
) has been added. It takes the current templates used in the Notifications database, and updates them to use the new URLs.These changes are now part of #3229.
Testing
CamelProcessorTest
implementers to use the new URLs in their Qute templatesdata.context.host_url
will always be used if presentExample
Using the following trimmed output from
BaseTransformer
:And the new Microsoft Teams template:
We generate this message: