-
Notifications
You must be signed in to change notification settings - Fork 114
fix: /ok-to-test /retest pipelineruns should not be created if last sha successful #2048
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,7 @@ The advantage of using a `GitOps command` is that it provides a journal of all t | |||||
|
||||||
## GitOps Commands on Pull Requests | ||||||
|
||||||
For example, when you are on a Pull Request, you may want to restart all your PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all PipelineRuns attached to that Pull Request will be restarted. | ||||||
For example, when you are on a Pull Request, you may want to restart failed PipelineRuns. To do so, you can add a comment on your Pull Request starting with `/retest`, and all **failed** PipelineRuns attached to that Pull Request will be restarted. If all previous PipelineRuns for the same commit were successful, no new PipelineRuns will be created to avoid unnecessary duplication. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @waveywaves here and all other places in docs you've updated, I see you're mentioning only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly because this PR is only related to /retest, I have updated all the tests to use /test instead of /retest whereever required. You want examples of of both /test and /retest through the repo is what you are saying ? |
||||||
|
||||||
Example: | ||||||
|
||||||
|
@@ -21,6 +21,23 @@ failure is not with your PR but seems to be an infrastructure issue. | |||||
/retest | ||||||
``` | ||||||
|
||||||
The `/retest` command will only create new PipelineRuns if: | ||||||
|
||||||
- Previously **failed** PipelineRuns for the same commit, OR | ||||||
- No PipelineRun has been run for the same commit yet | ||||||
|
||||||
If a successful PipelineRun already exists for the same commit, `/retest` will **skip** triggering a new PipelineRun to avoid unnecessary duplication. | ||||||
|
||||||
**To force a rerun regardless of previous status**, use: | ||||||
|
||||||
```text | ||||||
/retest <pipelinerun-name> | ||||||
``` | ||||||
|
||||||
This will always trigger a new PipelineRun, even if previous runs were successful. | ||||||
|
||||||
Similar to `/retest`, the `/ok-to-test` command will only trigger new PipelineRuns if no successful PipelineRun already exists for the same commit. This prevents duplicate runs when repository owners repeatedly test the same commit by `/test` and `/retest` command. | ||||||
|
||||||
If you have multiple `PipelineRun` and you want to target a specific `PipelineRun`, you can use the `/test` command followed by the specific PipelineRun name to restart it. Example: | ||||||
|
||||||
```text | ||||||
|
@@ -241,10 +258,10 @@ You can pass those `key=value` pairs anywhere in your comment, and they will be | |||||
|
||||||
There are different formats that can be accepted, allowing you to pass values with spaces or newlines: | ||||||
|
||||||
* key=value | ||||||
* key="a value" | ||||||
* key="another \"value\" defined" | ||||||
* key="another | ||||||
- key=value | ||||||
- key="a value" | ||||||
- key="another \"value\" defined" | ||||||
- key="another | ||||||
value with newline" | ||||||
|
||||||
## Event Type Annotation and Dynamic Variables | ||||||
|
@@ -253,14 +270,14 @@ The `pipeline.tekton.dev/event-type` annotation indicates the type of GitOps com | |||||
|
||||||
Here are the possible event types: | ||||||
|
||||||
* `test-all-comment`: The event is a single `/test` that would test every matched PipelineRun. | ||||||
* `test-comment`: The event is a `/test <PipelineRun>` comment that would test a specific PipelineRun. | ||||||
* `retest-all-comment`: The event is a single `/retest` that would retest every matched PipelineRun. | ||||||
* `retest-comment`: The event is a `/retest <PipelineRun>` that would retest a specific PipelineRun. | ||||||
* `on-comment`: The event is coming from a custom comment that would trigger a PipelineRun. | ||||||
* `cancel-all-comment`: The event is a single `/cancel` that would cancel every matched PipelineRun. | ||||||
* `cancel-comment`: The event is a `/cancel <PipelineRun>` that would cancel a specific PipelineRun. | ||||||
* `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user. | ||||||
- `test-all-comment`: The event is a single `/test` that would test every matched PipelineRun. | ||||||
- `test-comment`: The event is a `/test <PipelineRun>` comment that would test a specific PipelineRun. | ||||||
- `retest-all-comment`: The event is a single `/retest` that would retest every matched **failed** PipelineRun. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created. | ||||||
- `retest-comment`: The event is a `/retest <PipelineRun>` that would retest a specific PipelineRun. | ||||||
- `on-comment`: The event is coming from a custom comment that would trigger a PipelineRun. | ||||||
- `cancel-all-comment`: The event is a single `/cancel` that would cancel every matched PipelineRun. | ||||||
- `cancel-comment`: The event is a `/cancel <PipelineRun>` that would cancel a specific PipelineRun. | ||||||
- `ok-to-test-comment`: The event is a `/ok-to-test` that would allow running the CI for an unauthorized user. If a successful PipelineRun already exists for the same commit, no new PipelineRun will be created. | ||||||
|
||||||
If a repository owner comments `/ok-to-test` on a pull request from an external contributor but no PipelineRun **matches** the `pull_request` event (or the repository has no `.tekton` directory), Pipelines-as-Code sets a **neutral** commit status. This indicates that no PipelineRun was matched, allowing other workflows—such as auto-merge—to proceed without being blocked. | ||||||
|
||||||
|
@@ -272,12 +289,12 @@ Note: This neutral check-run status functionality is only supported on GitHub. | |||||
|
||||||
When using the `{{ event_type }}` [dynamic variable]({{< relref "/docs/guide/authoringprs.md#dynamic-variables" >}}) for the following event types: | ||||||
|
||||||
* `test-all-comment` | ||||||
* `test-comment` | ||||||
* `retest-all-comment` | ||||||
* `retest-comment` | ||||||
* `cancel-all-comment` | ||||||
* `ok-to-test-comment` | ||||||
- `test-all-comment` | ||||||
- `test-comment` | ||||||
- `retest-all-comment` | ||||||
- `retest-comment` | ||||||
- `cancel-all-comment` | ||||||
- `ok-to-test-comment` | ||||||
|
||||||
The dynamic variable will return `pull_request` as the event type instead of the specific categorized GitOps command type. This is to handle backward compatibility with previous releases for users relying on this dynamic variable. | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,8 +74,10 @@ get_tests() { | |
mapfile -t testfiles < <(find test/ -maxdepth 1 -name '*.go') | ||
ghglabre="Github|Gitlab|Bitbucket" | ||
if [[ ${target} == "providers" ]]; then | ||
# echo "TestGithubMaxKeepRuns" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @waveywaves I think this is added mistakenly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this when I was trying to test individually in the CI, I'll remove this in the very end |
||
grep -hioP "^func Test.*(${ghglabre})(\w+)\(" "${testfiles[@]}" | sed -e 's/func[ ]*//' -e 's/($//' | ||
elif [[ ${target} == "gitea_others" ]]; then | ||
elif [[ ${target} == "gitea_others" ]]; then | ||
# echo "TestGiteaParamsOnRepoCR" | ||
grep -hioP '^func Test(\w+)\(' "${testfiles[@]}" | grep -iPv "(${ghglabre})" | sed -e 's/func[ ]*//' -e 's/($//' | ||
else | ||
echo "Invalid target: ${target}" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |
apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" | ||
pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors" | ||
"github.com/openshift-pipelines/pipelines-as-code/pkg/events" | ||
"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" | ||
"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" | ||
"github.com/openshift-pipelines/pipelines-as-code/pkg/params" | ||
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" | ||
|
@@ -21,6 +22,8 @@ import ( | |
"github.com/google/cel-go/common/types" | ||
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" | ||
"go.uber.org/zap" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"knative.dev/pkg/apis" | ||
) | ||
|
||
const ( | ||
|
@@ -366,12 +369,120 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger | |
} | ||
|
||
if len(matchedPRs) > 0 { | ||
// Filter out templates that already have successful PipelineRuns for /retest and /ok-to-test | ||
if event.EventType == opscomments.RetestAllCommentEventType.String() || | ||
event.EventType == opscomments.OkToTestCommentEventType.String() { | ||
return filterSuccessfulTemplates(ctx, logger, cs, event, repo, matchedPRs), nil | ||
} | ||
return matchedPRs, nil | ||
} | ||
|
||
return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns)) | ||
} | ||
|
||
// filterSuccessfulTemplates filters out templates that already have successful PipelineRuns | ||
// when executing /ok-to-test or /retest gitops commands, implementing per-template checking. | ||
func filterSuccessfulTemplates(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository, matchedPRs []Match) []Match { | ||
if event.SHA == "" { | ||
return matchedPRs | ||
} | ||
|
||
// Get all existing PipelineRuns for this SHA | ||
labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA)) | ||
existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{ | ||
LabelSelector: labelSelector, | ||
}) | ||
if err != nil { | ||
logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err) | ||
return matchedPRs // Return all templates if we can't check | ||
} | ||
|
||
// Create a map of template names to their most recent successful run | ||
successfulTemplates := make(map[string]*tektonv1.PipelineRun) | ||
|
||
for i := range existingPRs.Items { | ||
pr := &existingPRs.Items[i] | ||
|
||
// Get the original template name this PipelineRun came from | ||
originalPRName, ok := pr.GetAnnotations()[keys.OriginalPRName] | ||
if !ok { | ||
originalPRName, ok = pr.GetLabels()[keys.OriginalPRName] | ||
} | ||
if !ok { | ||
continue // Skip PipelineRuns without template identification | ||
} | ||
|
||
// Check if this PipelineRun succeeded | ||
if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() { | ||
// Keep the most recent successful run for each template | ||
if existing, exists := successfulTemplates[originalPRName]; !exists || | ||
pr.CreationTimestamp.After(existing.CreationTimestamp.Time) { | ||
successfulTemplates[originalPRName] = pr | ||
} | ||
} | ||
} | ||
|
||
// Filter out templates that have successful runs | ||
var filteredPRs []Match | ||
var anySkipped bool | ||
|
||
for _, match := range matchedPRs { | ||
templateName := getName(match.PipelineRun) | ||
|
||
if successfulPR, hasSuccessfulRun := successfulTemplates[templateName]; hasSuccessfulRun { | ||
logger.Infof("skipping template '%s' for sha %s as it already has a successful pipelinerun '%s'", | ||
templateName, event.SHA, successfulPR.Name) | ||
anySkipped = true | ||
} else { | ||
filteredPRs = append(filteredPRs, match) | ||
} | ||
} | ||
|
||
// If all templates were skipped, return a reference to the first successful one | ||
if len(filteredPRs) == 0 && anySkipped && len(successfulTemplates) > 0 { | ||
// Return any successful run as reference | ||
for _, successfulPR := range successfulTemplates { | ||
return []Match{{PipelineRun: successfulPR, Repo: repo}} | ||
} | ||
} | ||
Comment on lines
+441
to
+447
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current logic of returning an existing successful To fix this, you should return an empty slice, which will prevent any new // If all templates were skipped, return an empty slice to prevent new runs.
if len(filteredPRs) == 0 && anySkipped {
logger.Infof("all matched pipelineruns for sha %s have already succeeded, skipping", event.SHA)
return []Match{}
} |
||
|
||
return filteredPRs | ||
} | ||
|
||
// checkForExistingSuccessfulPipelineRun is a helper function for testing. | ||
// It checks if there's an existing successful PipelineRun for the same SHA. | ||
func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun { | ||
// Only check for /retest and /ok-to-test commands | ||
if event.EventType != opscomments.RetestAllCommentEventType.String() && | ||
event.EventType != opscomments.OkToTestCommentEventType.String() || | ||
waveywaves marked this conversation as resolved.
Show resolved
Hide resolved
|
||
event.SHA == "" { | ||
return nil | ||
} | ||
|
||
labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA)) | ||
existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{ | ||
LabelSelector: labelSelector, | ||
}) | ||
if err != nil { | ||
logger.Errorf("failed to list existing PipelineRuns for SHA %s: %v", event.SHA, err) | ||
return nil | ||
} | ||
|
||
// Find the most recent successful PipelineRun | ||
var mostRecentSuccessfulPR *tektonv1.PipelineRun | ||
for i := range existingPRs.Items { | ||
pr := &existingPRs.Items[i] | ||
if pr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() { | ||
if mostRecentSuccessfulPR == nil || | ||
pr.CreationTimestamp.After(mostRecentSuccessfulPR.CreationTimestamp.Time) { | ||
mostRecentSuccessfulPR = pr | ||
} | ||
} | ||
} | ||
|
||
return mostRecentSuccessfulPR | ||
} | ||
|
||
func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string { | ||
errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:" | ||
for _, prun := range pruns { | ||
|
Uh oh!
There was an error while loading. Please reload this page.