Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/packagekit/mock-updates.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,13 @@ export function injectMockUpdates(updates) {
severity: 6,
description: "This is FUBAR",
};

updates["tracker-links;1-1"] = {
name: "tracker-links",
version: "1-1",
bug_urls: [],
cve_urls: [],
severity: 4,
description: "Fixes rhbz#12345 and jira#67890 in one update.",
};
}
46 changes: 45 additions & 1 deletion pkg/packagekit/updates.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,49 @@ function cleanupChangelogLine(text) {
return text.trim();
}

function isRedHatBasedDistribution() {
const os_release = cockpit.info?.os_release;
if (!os_release)
return false;

const ids = [os_release.ID, ...(os_release.ID_LIKE || "").split(" ")].filter(Boolean);
return ids.some(id => ["rhel", "fedora", "centos"].includes(id));
}

function renderChangelogWithIssueLinks(text) {
if (!text || !isRedHatBasedDistribution())
return text;

const issuePattern = /(rhbz|jira)#(\d+)/g;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can check for rhbz no matter the distro tbh. I don't think anyone else refers to their trackers as rhbz and it is quite normal to link to in regards to CVEs etc.

However, with Jira I agree that we should only link to it within RHEL-sphere, but is it common to write jira#1234? That feels very strange to me as it doesn't include the namespace at all like RHEL-1234.

Do you have links to where the jira#1234 syntax is used?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn’t find solid evidence that jira#1234 is being used; most Red Hat references I found are RHEL-1234. Should I drop the Jira part and add something like RHEL-1234?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we use RHEL-1234 there are many many other projects in Jira, not to mention other Jira instances from other companies might have similar naming. Could you give the RH refs you found for RHEL-1234?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Then lets go with that for now.

So I'd request to make

  1. RHBZ linking not specific to Fedora-based distros
  2. Add checking for RHEL-\d+ for Fedora-based distros

const result = [];
let lastIndex = 0;
let match;

while ((match = issuePattern.exec(text)) !== null) {
if (match.index > lastIndex)
result.push(text.slice(lastIndex, match.index));

const [, tracker, issueId] = match;
const href = tracker === "rhbz"
? `https://bugzilla.redhat.com/show_bug.cgi?id=${issueId}`
: `https://issues.redhat.com/browse/${issueId}`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be https://redhat.atlassian.net

result.push(
<a key={`${tracker}-${issueId}-${match.index}`} href={href} rel="noopener noreferrer" target="_blank">
{match[0]}
</a>
);
lastIndex = issuePattern.lastIndex;
}

if (lastIndex === 0)
return text;

if (lastIndex < text.length)
result.push(text.slice(lastIndex));

return result;
}

// Replace cockpit-wsinstance-https@[long_id] with a shorter string
function shortenCockpitWsInstance(list) {
return list.map(item => item.startsWith('cockpit-wsinstance-https') ? 'cockpit-wsinstance-https@.' : item);
Expand Down Expand Up @@ -323,7 +366,8 @@ function updateItem(remarkable, info, pkgNames, key) {
descriptionFirstLine = <span dangerouslySetInnerHTML={{ __html: remarkable.render(descriptionFirstLine) }} />;
description = <div dangerouslySetInnerHTML={{ __html: remarkable.render(info.description) }} />;
} else {
description = <div className="changelog">{info.description}</div>;
descriptionFirstLine = renderChangelogWithIssueLinks(descriptionFirstLine);
description = <div className="changelog">{renderChangelogWithIssueLinks(info.description)}</div>;
Comment on lines 381 to +383

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking this out now to see how it would look. There is one issue that makes me think we need to modify description for entries with markdown as well since these otherwise don't get changes. Which does create a bit of a jarring experience where some has links and some don't, or right out doesn't work at all as all changelog entries are markdown for the distro (feels like it on Fedora based on the results I get)

Image

While we can't change everything to markdown as it would break for some packages that are supposed to be plaintext, we could modify the markdown or the HTML generated from markdown as long as it doesn't match

  • [rhbz#1234](http...)
  • [rhbz#1234 'aria label'](http...)
  • <a href="http..." aria-label="aria label">rhbz#1234</a>

Probably the easiest is to detect if it's DOM in renderChangelogWithIssueLinks and in that case check that the text matched isn't directly within an anchor element. Then again matching wouldn't be that easy without converting to DOM first and searching for it I think..

@mvollmer WDYT?

}

const expandedContent = (
Expand Down