Summary of Changes

Hello @theakshaypant, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the GitLab integration by introducing an "update" comment strategy. This new feature streamlines communication on merge requests by consolidating all PipelineRun status updates and CEL validation errors into a single, continuously updated comment. This approach reduces comment clutter and provides a clear, real-time overview of pipeline execution directly within the merge request interface.

Highlights

• New GitLab Comment Strategy: Introduced an "update" comment strategy for GitLab, allowing a single comment per PipelineRun to be updated on every trigger. This prevents comment spam and provides a consolidated view of pipeline status.
• Enhanced CEL Validation Error Reporting: When the "update" comment strategy is active, Common Expression Language (CEL) validation errors are now reported immediately within the single updatable comment, using a PipelineRun-specific prefix for clarity.
• API and CRD Updates: The CommentStrategy field in GitlabSettings within both the Custom Resource Definition (CRD) and the Go type definition has been extended to include the new "update" option, making it a configurable choice.
• Standardized Comment Formatting: A new internal helper function, formatPipelineComment, was added to generate standardized, emoji-rich status comments for PipelineRuns, ensuring consistent and easily understandable messages.

🔍 PR Lint Feedback

Note: This automated check helps ensure your PR follows our contribution guidelines.

⚠️ Items that need attention:

🤖 AI attribution

The following commits lack an explicit AI attribution footer:

• 922bb39 feat(gitlab): add update comment stategy

If no AI assistance was used for a commit, you can ignore this warning.
Otherwise add an Assisted-by: or Co-authored-by: footer referencing the AI used.

ℹ️ Next Steps

• Review and address the items above
• Push new commits to update this PR
• This comment will be automatically updated when issues are resolved

_{🤖 This feedback was generated automatically by the PR CI system}

/review

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

func reportCELValidationErrors(ctx context.Context, repo *apipac.Repository, validationErrors []*pacerrors.PacYamlValidations, eventEmitter *events.EventEmitter, vcx provider.Interface, event *info.Event, prefix string) {
	errorRows := make([]string, 0, len(validationErrors))
	for _, err := range validationErrors {
		errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", err.Name, err.Err.Error()))
	}
	if len(errorRows) == 0 {
		return
	}

	var err error

	// The prefix is used for a single comment "update" strategy where the same comment is updated
	// with the celValidationError and the PipelineRun status for any subsequent or prior "runs".
	if prefix != "" {
		markdownErrMessage := fmt.Sprintf(`%s
%s
%s`, prefix, provider.ValidationErrorTemplate, strings.Join(errorRows, "\n"))
		err = vcx.CreateComment(ctx, event, markdownErrMessage, prefix)
	} else {
		markdownErrMessage := fmt.Sprintf(`%s
%s`, provider.ValidationErrorTemplate, strings.Join(errorRows, "\n"))
		err = vcx.CreateComment(ctx, event, markdownErrMessage, provider.ValidationErrorTemplate)
	}
	if err != nil {
		eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError",
			fmt.Sprintf("failed to create comment: %s", err.Error()))
	}

	if provider.IsCommentStrategyUpdate(repo) {
		reportCELValidationErrors(ctx, repo, []*pacerrors.PacYamlValidations{{
			Name: prName,
			Err:  fmt.Errorf("CEL expression evaluation error: %s", sanitizeErrorAsMarkdown(err)),
		}}, eventEmitter, vcx, event, fmt.Sprintf(provider.PlrStatusCommentPrefixTemplate, prName))
	} else {
		celValidationErrors = append(celValidationErrors, &pacerrors.PacYamlValidations{
			Name: prName,
			Err:  fmt.Errorf("CEL expression evaluation error: %s", sanitizeErrorAsMarkdown(err)),
		})
	}
}

case provider.UpdateCommentStrategy:
	if eventType == triggertype.PullRequest || provider.Valid(event.EventType, anyMergeRequestEventType) {
		statusComment := v.formatPipelineComment(event.SHA, statusOpts)
		// Creating the prefix that is added to the status comment for a pipeline run.
		plrStatusCommentPrefix := fmt.Sprintf(provider.PlrStatusCommentPrefixTemplate, statusOpts.OriginalPipelineRunName)
		// The entire markdown comment, including the prefix that is added to the pull request for the pipelinerun.
		markdownStatusComment := fmt.Sprintf("%s\n%s", plrStatusCommentPrefix, statusComment)

		if err := v.CreateComment(ctx, event, markdownStatusComment, plrStatusCommentPrefix); err != nil {
			v.eventEmitter.EmitMessage(
				v.repo,
				zap.ErrorLevel,
				"PipelineRunCommentCreationError",
				fmt.Sprintf("failed to create comment: %s", err.Error()),
			)
			return err
		}
	}

... Verify ... prefix is stable/unique across retries and name changes (e.g., if statusOpts.OriginalPipelineRunName differs from prName in some paths).

This is a good catch, addressed in ab9acbe

❌ PR-Agent failed to apply 'global' repo settings

The configuration file needs to be a valid TOML, please fix it.

Error message:
cannot access local variable 'file_data' where it is not associated with a value

[github_app]
pr_commands = [ "/review" ]

[pr_reviewer]
enable_review_labels_security = true
enable_chat_in_code_suggestions=false
enable_chat_in_code_suggestions = false
enable_summary=false
enable_help_text=false

[pr_description]
publish_description_as_comment = true

[pr_code_suggestions]
commitable_code_suggestions = true

[pr_test]
num_tests = 0

if it's ready for review, can you remove the draft?

if it's ready for review, can you remove the draft?

Will update with changes from this week and move it to ready.

I like this direction overall.

• Dropping comment-ID caching sounds like the right tradeoff for now..
• I also agree with the UX change for comment_strategy: update showing only the latest PLR state (CEL error -> running/success/failure) is cleaner than leaving stale CEL error comments around.

One thing I’d like us to fix before merge:

• when dong the comment update matching, we currently compile the marker directly as regex. Since marker includes PLR name, regex-significant characters can make incorrect matches. Can we switch to regexp.QuoteMeta(updateMarker) before regexp.MustCompile(...)? (ie:
  would mismatch to and others).

Test coverage I’d like to see but skip it if they are too hard to implement for the values they gives:

• CEL error is posted for a PLR, then replaced by status comment after the CEL issue is fixed.
• Multiple PLRs in one MR each update only their own comment (no cross-updates).
• Marker matching remains exact even when PLR names contain regex-relevant characters.

when dong the comment update matching, we currently compile the marker directly as regex. Since marker includes
PLR name, regex-significant characters can make incorrect matches. Can we switch to
regexp.QuoteMeta(updateMarker) before regexp.MustCompile(...)? (ie:
would mismatch to and others).

That's a good point, will make this change.

Test coverage I’d like to see but skip it if they are too hard to implement for the values they gives:

* CEL error is posted for a PLR, then replaced by status comment after the CEL issue is fixed.

* Multiple PLRs in one MR each update only their own comment (no cross-updates).

* Marker matching remains exact even when PLR names contain regex-relevant characters.

I was planning on adding the first 2 cases you mentioned but for github. Since comments are fallback on gitlab, I thought it would be easier to have this in e2e tests for github provider using webhook.