Enable relative TaskRefs within remote Pipelines

[martindbrv]

Description

The current implementation doesn't allow to reference tasks relative to a remote pipeline (e.g., the task definition is in the same remote location as the pipeline). The issue is described here:

https://issues.redhat.com/browse/SRVKP-4332

Changes

This PR includes a simple function that makes use of the task overriding functionality (see the relative path example).
Previously, only relative overrides for tasks within the triggering event's .tekton folder were supported. Now, it simply checks if there is a pipelinesascode.tekton.dev/task with a relative path value in the remote pipeline's definition and then assembles the tasks's FQDN based on the remote pipeline's location.

Consider the following scenario:

Repository A, contains:

• .tekton/pipelinerun.yaml

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  name: hello-world
  annotations:
    pipelinesascode.tekton.dev/on-target-branch: "[main]"
    pipelinesascode.tekton.dev/on-event: "[push]"
    pipelinesascode.tekton.dev/pipeline: "https://github.com/user/repositoryb/blob/main/pipeline.yaml"
spec:
  pipelineRef:
    name: hello-world

Repository B, contains:

• pipeline.yaml

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: hello-world
  annotations:
    pipelinesascode.tekton.dev/task: "./task.yaml"
spec:
  tasks:
    - name: say-hello
      taskRef:
        name: hello

• task.yaml

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  steps:
    - name: echo
      image: alpine
      script: |
        #!/bin/sh
        echo "Hello, World!"

Previously, you had to specify the FQDN of the task either within the PipelineRun or the Pipeline. Now, you can still do that but you can also use relative references as shown above in the hello-world pipeline.

Note

I will modify the docs to include info about the new functionality. I will also write some additional tests.
Just wanted to get some initial feedback before doing these things.

Submitter Checklist

• 📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).

• ♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.

• ✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).

• 📖 Document any user-facing features or changes in behavior.

• 🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.

• 🎁 If feasible, add an end-to-end test. See README for details.

• 🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).

• If adding a provider feature, fill in the following details:

  • GitHub App
  • GitHub Webhook
  • Gitea/Forgejo
  • GitLab
  • Bitbucket Cloud
  • Bitbucket Data Center

[chmouel]

/ok-to-test

[Copilot]

Pull Request Overview

This PR enables relative task references in remote pipeline definitions by constructing fully qualified URLs based on the remote pipeline's location.

• Adds a new function, createTaskURL, to transform relative task paths into absolute URLs.
• Updates resolveRemoteResources to use createTaskURL so that pipelines declared remotely can reference tasks relative to their own location.

[chmouel]

Hey you PR make sense, but we need to review carefully for side effects, will have a review on this week

[chmouel]

/ok-to-test

[chmouel]

🚀 Mirrored PR Created for E2E Testing

A mirrored PR has been opened for end-to-end testing: View PR

⏳ Follow progress there for E2E results.
If you need to update the PR with new changes, please ask a maintainer to rerun hack/mirror-pr.sh.

[chmouel]

🤖 AI Review provided by gemini:gemini-2.5-pro:

Of course! This is an excellent way to review code. I will carefully analyze the three commits, their evolution, and the final state of the code.

High-Level Summary

This is a well-structured pull request that adds a valuable feature: resolving relative task paths within a remote pipeline definition. The series of three commits shows a good iterative development process:

1. PATCH 1/3: Introduces the core functionality.
2. PATCH 2/3: Fixes a bug in the initial implementation where URLs were being incorrectly concatenated.
3. PATCH 3/3: Fixes a more subtle but critical bug related to non-deterministic behavior caused by Go's map iteration order, while also making a small code improvement.

The commit messages are clear and explain the "why" behind each change. The PR description provides excellent context and a clear example.

Detailed Code Analysis

I'll focus my analysis on the final state of the code after all three patches are applied, as that's what will be merged. I will also comment on the evolution of the code.

Final Code (after PATCH 3/3)

// file: pkg/resolve/remote.go

func createTaskURL(eventPipelines map[string]*tektonv1.Pipeline, pipeline *tektonv1.Pipeline, tasks []string) ([]string, error) {
	var pURL *url.URL
	var err error
	for ep := range eventPipelines {
		// ensure the URL to be parsed matches the
		// remote pipeline of the current PipelineRun
		if eventPipelines[ep] != pipeline {
			continue
		}
		pURL, err = url.Parse(ep)
		if err != nil {
			return tasks, err
		}
		pPath := strings.SplitAfter(pURL.Path, "/")
		// pop the pipeline target path from the URL
		pPath = pPath[:len(pPath)-1]
		pURL.Path = strings.Join(pPath, "")
	}
	taskURLS := make([]string, len(tasks))
	for i, t := range tasks {
		tURL, err := url.Parse(t)
		if err == nil && tURL.Scheme != "" && tURL.Host != "" {
			taskURLS[i] = t
			continue // it's already an absolute URL
		}
		tURL = pURL
		tURL = tURL.JoinPath(t)
		taskURLS[i] = tURL.String()
	}
	return taskURLS, nil
}

---

🐛 Bug Analysis

I have found one potential bug in the final version of the code.

Potential Nil Pointer Dereference Panic

In the createTaskURL function, the first for loop is responsible for finding the URL of the current pipeline and parsing it into pURL.

	var pURL *url.URL
	var err error
	for ep := range eventPipelines {
		if eventPipelines[ep] != pipeline {
			continue
		}
		pURL, err = url.Parse(ep)
        //...
	}

If the pipeline object is not found in the eventPipelines map for any reason, the loop will complete and pURL will remain nil. The code will then proceed to the second loop:

	for i, t := range tasks {
        // ...
		tURL = pURL // tURL is now nil
		tURL = tURL.JoinPath(t) // This will cause a panic: nil pointer dereference
		taskURLS[i] = tURL.String()
	}

Recommendation: Add a check after the first loop to ensure pURL was successfully assigned.

// ... after the first for loop
	if pURL == nil {
		// This case should ideally not happen if the logic is correct elsewhere,
		// but it's a necessary safeguard.
		return nil, fmt.Errorf("could not find the remote URL for pipeline %s", pipeline.Name)
	}
// ... continue to the second for loop

---

💡 Code Improvements & Idiomatic Go

The code is functional, but it could be made more robust and idiomatic.

1. Simplifying Path Manipulation

The current code manually manipulates the URL path to get the parent directory:

pPath := strings.SplitAfter(pURL.Path, "/")
pPath = pPath[:len(pPath)-1]
pURL.Path = strings.Join(pPath, "")

This can be done more cleanly and expressively using the standard path package. The path.Dir function is designed for exactly this purpose.

Recommendation: Replace the three lines above with a single, more readable line.

import (
    "path" // Make sure to add this import
    // ...
)

// ... inside the loop
pURL.Path = path.Dir(pURL.Path)

This is more idiomatic and less prone to off-by-one errors than manual slice manipulation.

2. Function Naming

The function is named createTaskURL. However, its primary job is to resolve relative task paths into fully-qualified URLs. It doesn't always "create" them, as it passes through absolute URLs unchanged.

Recommendation: Consider renaming the function to something more descriptive of its action, such as resolveRelativeTaskURLs or makeTaskURLsAbsolute. This makes the code's intent clearer to future readers.

3. Inefficient Loop to Find the Pipeline URL

The code iterates over the eventPipelines map to find the key (URL) for a given value (pipeline object).

for ep := range eventPipelines {
    if eventPipelines[ep] != pipeline {
        continue
    }
    // ... found it
}

This is an O(n) operation. While likely not a performance bottleneck given the small number of pipelines expected per event, it's worth noting. If the calling function resolveRemoteResources could construct a reverse map (map[*tektonv1.Pipeline]string) or pass the pipeline's URL directly, this loop could be avoided. However, given the existing data structures, this is a necessary lookup. The fix for the non-determinism in PATCH 3/3 is excellent, and this implementation is a correct way to achieve it.

---

✅ Spelling and Grammar

The commit messages and PR description are very well-written. I found only one extremely minor typo in the PR description text.

• In the PR description: "...assembles the tasks's FQDN..."
• Correction: Should be "...assembles the task's FQDN..." (for a single task) or "...assembles the tasks' FQDNs..." (for multiple tasks).

Conclusion

This is a solid contribution. The author correctly identified the need for this feature and navigated a couple of tricky implementation details (URL accumulation and map iteration order) to arrive at a working solution.

Summary of Recommendations:

1. Critical: Fix the potential nil pointer panic by adding a check after the first loop in createTaskURL.
2. Highly Recommended: Improve code clarity and make it more idiomatic by using path.Dir for path manipulation.
3. Suggested: Consider renaming createTaskURL to better reflect its purpose (e.g., resolveRelativeTaskURLs).
4. Minor: Correct the minor grammatical error in the PR description.

After addressing the nil pointer bug, this change will be a great addition to the codebase.