Skip to content

Add link to GitHub pull request#80

Open
clayburn wants to merge 3 commits intomainfrom
cj/pr-link
Open

Add link to GitHub pull request#80
clayburn wants to merge 3 commits intomainfrom
cj/pr-link

Conversation

@clayburn
Copy link
Copy Markdown
Member

@clayburn clayburn commented Mar 6, 2026

Summary

Test plan

  • Verify the build compiles and existing tests pass
  • Trigger a GitHub Actions PR build and confirm the "GitHub pull request" link appears in the Build Scan

When building on GitHub Actions for a pull request, add a "GitHub pull
request" link to the Build Scan by extracting the PR number from the
GITHUB_REF_NAME environment variable.

See gradle/common-custom-user-data-gradle-plugin#472
@clayburn clayburn requested a review from a team March 6, 2026 16:13
@clayburn clayburn requested review from Duhemm and dotta April 3, 2026 14:19
Copy link
Copy Markdown
Member

@dotta dotta left a comment

Choose a reason for hiding this comment

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

Shared my feedback, but I'm leaving it to @Duhemm to accept or request changes.

val pullRequestUrl = for {
url <- serverUrl
repository <- gitRepository
_ <- headRef
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.

Why is it required for headRef to be Some? If it's indeed required, I'd lift this out and make it a conditional for the whole expression, i.e.,

if (headRef.nonEmpty) {
  val pullRequestUrl = for { ... 
}

This because I believe it makes the intended logic a lot more apparent and easier to reason about.

Copy link
Copy Markdown
Member

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

I think it might make sense to move the PR number extraction logic to a dedicated method so that it's obvious that it'll return something only when we're running in a PR. That'll also address Mirco's concern about the guard being a bit obscure.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants