Skip to content

feat(STONEINTG-1546): add plr link to git report#1613

Open
ainephelan365 wants to merge 1 commit into
konflux-ci:mainfrom
ainephelan365:feat/STONEINTG-1546-plr-link
Open

feat(STONEINTG-1546): add plr link to git report#1613
ainephelan365 wants to merge 1 commit into
konflux-ci:mainfrom
ainephelan365:feat/STONEINTG-1546-plr-link

Conversation

@ainephelan365

@ainephelan365 ainephelan365 commented Jul 1, 2026

Copy link
Copy Markdown

-created clickable url link in status.go

  • added unit test

Maintainers will complete the following section

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the pipelineRun retrieval logic to format and include a pipeline URL in the fallback message when a pipelineRun is not found, and adds a corresponding unit test. However, the implementation replaces the general error check with a specific 'IsNotFound' check, which silently ignores other API errors and allows the function to proceed with an unpopulated pipelineRun. It is recommended to restore the general error handling to prevent unexpected behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread status/status.go Outdated
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:00 AM UTC · Completed 10:10 AM UTC
Commit: ec21706 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [markdown formatting consistency] status/status.go:306 — The error message uses markdown-style links [text](url) while HTML templates in format.go use <a href> tags. However, the HTML links exist within Go HTML templates that render structured table/list markup, whereas this error message is a plain fmt.Sprintf string rendered in a markdown context (GitHub/GitLab status descriptions). Markdown link syntax is contextually appropriate here, so this is a minor style observation rather than a bug.
Previous run

Review

Findings

Low

  • [Message consistency] status/status.go:306 — The error message uses markdown link syntax [name](url) while the rest of the codebase (status/format.go templates) uses HTML <a href=...> tags for links. Both syntaxes render correctly in GitHub contexts, but this introduces a minor style inconsistency.
    Remediation: Consider changing the link format from [%s](%s) to <a href="%s">%s</a> to match the established HTML anchor pattern used in status/format.go.

  • [Error message grammar] status/status.go:306 — The error message uses "can not" (two words). "cannot" (one word) is the standard form in technical writing. Note: this pre-dates this PR but is carried forward in the changed line.
    Remediation: Change "can not be found" to "cannot be found".


Labels: PR adds a UX enhancement (clickable PipelineRun link in error messages).

Previous run (2)

Review

Findings

Low

  • [link-format-convention] status/status.go:305 — The error message uses Markdown link format [text](url), while templates in format.go use HTML anchor tags (<a href>). Both formats render correctly in GitHub Check Runs and PR comments, so this is a stylistic inconsistency rather than a bug. Optional: align to HTML anchor format for consistency with the rest of the file.
  • [grammar-consistency] status/status.go:305 — The message uses "can not" (two words); the file has mixed usage (line 571: "can not", line 641: "cannot"). This is a pre-existing inconsistency, not introduced by this PR.
Previous run (3)

Looks good to me

Previous run (4)

Review

Findings

High

  • [error-handling-gap] status/status.go:302 — The diff removes the outer if err != nil guard and the generic error return that previously caught all non-NotFound errors from client.Get. After this change, if client.Get returns any error other than NotFound (e.g., network timeout, RBAC/forbidden, internal server error), apierrors.IsNotFound(err) evaluates to false, so the code falls through and proceeds to call helpers.GetAllChildTaskRunsForPipelineRun with an uninitialized/empty pipelineRun object. This will either produce misleading results or a confusing downstream error instead of the clear, actionable error that existed before.
    Remediation: Restore the outer if err != nil guard. The new NotFound handling (with the URL link) should remain nested inside it: if err != nil { if apierrors.IsNotFound(err) { ... } return "", fmt.Errorf("error while getting the pipelineRun %s: %w", pipelineRunName, err) }.

Low

  • [pattern-inconsistency] status/status_test.go:887 — Comment immediately before the It() block is slightly unusual for this file — most It() blocks in this file lack a preceding comment. Consider removing the comment and letting the test description be self-documenting.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:48 AM UTC · Completed 11:58 AM UTC
Commit: ec21706 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot dismissed their stale review July 1, 2026 11:58

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jul 1, 2026
@ainephelan365 ainephelan365 force-pushed the feat/STONEINTG-1546-plr-link branch from 35b967e to a8c2ee4 Compare July 1, 2026 14:47
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:52 PM UTC · Completed 3:03 PM UTC
Commit: ec21706 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jul 1, 2026
@codecov-commenter

codecov-commenter commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.38%. Comparing base (8f9e64f) to head (1b8169a).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
+ Coverage   65.85%   70.38%   +4.52%     
==========================================
  Files          63       71       +8     
  Lines        8725    11419    +2694     
==========================================
+ Hits         5746     8037    +2291     
- Misses       2306     2571     +265     
- Partials      673      811     +138     
Flag Coverage Δ
e2e-tests 47.56% <0.00%> (?)
unit-tests 67.37% <100.00%> (+1.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
status/status.go 68.77% <100.00%> (+10.36%) ⬆️

... and 55 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f9e64f...1b8169a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ainephelan365 ainephelan365 force-pushed the feat/STONEINTG-1546-plr-link branch from a8c2ee4 to 17e4f76 Compare July 1, 2026 20:40
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:43 PM UTC · Completed 8:53 PM UTC
Commit: ec21706 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge enhancement New feature or request and removed ready-for-merge All reviewers approved — ready to merge labels Jul 1, 2026

@jsztuka jsztuka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm, good job!

Handle missing pipelinerun gracefully by providing a linkable name.

Signed-off-by: aphelan <aphelan@redhat.com>
@ainephelan365 ainephelan365 force-pushed the feat/STONEINTG-1546-plr-link branch from 17e4f76 to 1b8169a Compare July 2, 2026 08:24
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:28 AM UTC · Completed 8:36 AM UTC
Commit: ec21706 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jul 2, 2026
@jsztuka

jsztuka commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

/ok-to-test

@NigelByrne1

Copy link
Copy Markdown
Contributor

/retest

@NigelByrne1 NigelByrne1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kasemAlem

Copy link
Copy Markdown
Contributor

well done, thanks @ainephelan365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ready-for-merge All reviewers approved — ready to merge size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants