Promote pull request properties (#5169)

* Promote pull request properties The source and target PR information, as well as the upstream URL have been promoted to top level properties. The GitLab provider now fills these in by default and uses them to populate the PR protobuf. The GitHub provider fills in the properties in, but currently does not leverage them; this will be done after some rollout time. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com> * Fix target/source confusion Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com> * Enhance logs. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com> --------- Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
mindersec · Dec 10, 2024 · ad08eec · ad08eec
1 parent a0d9e90
commit ad08eec
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 32 deletions.
diff --git a/internal/entities/properties/constants.go b/internal/entities/properties/constants.go
@@ -21,6 +21,24 @@ const (
 	RepoPropertyIsFork = "is_fork"
 )
 
+// Pull Request property keys
+const (
+	// PullRequestCommitSHA represents the commit SHA of the pull request
+	PullRequestCommitSHA = "commit_sha"
+	// PullRequestBaseCloneURL represents the clone URL of the base repository
+	PullRequestBaseCloneURL = "base_clone_url"
+	// PullRequestBaseDefaultBranch represents the default branch of the base repository
+	PullRequestBaseDefaultBranch = "base_default_branch"
+	// PullRequestTargetCloneURL represents the clone URL of the target repository.
+	// Where the pull request comes from.
+	PullRequestTargetCloneURL = "target_clone_url"
+	// PullRequestTargetBranch represents the default branch of the target repository.
+	// Where the pull request comes from.
+	PullRequestTargetBranch = "target_branch"
+	// PullRequestUpstreamURL represents the URL of the pull request in the provider
+	PullRequestUpstreamURL = "upstream_url"
+)
+
 // Artifact property keys
 const (
 	// ArtifactPropertyType represents the type of the artifact (e.g 'container')

diff --git a/internal/providers/github/properties/pull_request.go b/internal/providers/github/properties/pull_request.go
@@ -52,6 +52,12 @@ var prPropertyDefinitions = []propertyOrigin{
 			// general entity
 			properties.PropertyName,
 			properties.PropertyUpstreamID,
+			properties.PullRequestCommitSHA,
+			properties.PullRequestBaseCloneURL,
+			properties.PullRequestBaseDefaultBranch,
+			properties.PullRequestTargetCloneURL,
+			properties.PullRequestTargetBranch,
+			properties.PullRequestUpstreamURL,
 			// github-specific
 			PullPropertyURL,
 			PullPropertyNumber,
@@ -150,8 +156,14 @@ func getPrWrapper(
 
 	prProps := map[string]any{
 		// general entity
-		properties.PropertyUpstreamID: properties.NumericalValueToUpstreamID(prReply.GetID()),
-		properties.PropertyName:       fmt.Sprintf("%s/%s/%d", owner, name, intId),
+		properties.PropertyUpstreamID:           properties.NumericalValueToUpstreamID(prReply.GetID()),
+		properties.PropertyName:                 fmt.Sprintf("%s/%s/%d", owner, name, intId),
+		properties.PullRequestCommitSHA:         prReply.GetHead().GetSHA(),
+		properties.PullRequestBaseCloneURL:      prReply.GetBase().GetRepo().GetCloneURL(),
+		properties.PullRequestBaseDefaultBranch: prReply.GetBase().GetRepo().GetDefaultBranch(),
+		properties.PullRequestTargetCloneURL:    prReply.GetHead().GetRepo().GetCloneURL(),
+		properties.PullRequestTargetBranch:      prReply.GetHead().GetRef(),
+		properties.PullRequestUpstreamURL:       prReply.GetHTMLURL(),
 		// github-specific
 		PullPropertyURL: prReply.GetHTMLURL(),
 		// our proto representation uses int64 for the number but GH uses int

diff --git a/internal/providers/gitlab/properties.go b/internal/providers/gitlab/properties.go
@@ -38,16 +38,8 @@ const (
 	PullRequestProjectID = "gitlab/project_id"
 	// PullRequestNumber represents the gitlab merge request number
 	PullRequestNumber = "gitlab/merge_request_number"
-	// PullRequestSourceBranch represents the gitlab source branch
-	PullRequestSourceBranch = "gitlab/source_branch"
-	// PullRequestTargetBranch represents the gitlab target branch
-	PullRequestTargetBranch = "gitlab/target_branch"
 	// PullRequestAuthor represents the gitlab author
 	PullRequestAuthor = "gitlab/author"
-	// PullRequestCommitSHA represents the gitlab commit SHA
-	PullRequestCommitSHA = "gitlab/commit_sha"
-	// PullRequestURL represents the gitlab merge request URL
-	PullRequestURL = "gitlab/merge_request_url"
 )
 
 // Release Properties

diff --git a/internal/providers/gitlab/pull_request_properties.go b/internal/providers/gitlab/pull_request_properties.go
@@ -25,6 +25,7 @@ func FormatPullRequestUpstreamID(id int) string {
 	return fmt.Sprintf("%d", id)
 }
 
+//nolint:gocyclo // TODO: Refactor to reduce complexity
 func (c *gitlabClient) getPropertiesForPullRequest(
 	ctx context.Context, getByProps *properties.Properties,
 ) (*properties.Properties, error) {
@@ -87,15 +88,24 @@ func (c *gitlabClient) getPropertiesForPullRequest(
 		return nil, fmt.Errorf("failed to get project: %w", err)
 	}
 
-	outProps, err := gitlabMergeRequestToProperties(mr, proj)
+	targetproj := proj
+	if mr.SourceProjectID != 0 && mr.SourceProjectID != proj.ID {
+		targetproj, err = c.getGitLabProject(ctx, FormatRepositoryUpstreamID(mr.SourceProjectID))
+		if err != nil {
+			return nil, fmt.Errorf("failed to get target project: %w", err)
+		}
+	}
+
+	outProps, err := gitlabMergeRequestToProperties(mr, proj, targetproj)
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert merge request to properties: %w", err)
 	}
 
 	return outProps, nil
 }
 
-func gitlabMergeRequestToProperties(mr *gitlab.MergeRequest, proj *gitlab.Project) (*properties.Properties, error) {
+func gitlabMergeRequestToProperties(
+	mr *gitlab.MergeRequest, proj *gitlab.Project, targetproj *gitlab.Project) (*properties.Properties, error) {
 	ns, err := getGitlabProjectNamespace(proj)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get namespace: %w", err)
@@ -105,18 +115,20 @@ func gitlabMergeRequestToProperties(mr *gitlab.MergeRequest, proj *gitlab.Projec
 
 	outProps, err := properties.NewProperties(map[string]any{
 		// Unique upstream ID for the merge request
-		properties.PropertyUpstreamID: FormatPullRequestUpstreamID(mr.ID),
-		properties.PropertyName:       formatPullRequestName(ns, projName, FormatPullRequestUpstreamID(mr.IID)),
-		RepoPropertyNamespace:         ns,
-		RepoPropertyProjectName:       projName,
+		properties.PropertyUpstreamID:           FormatPullRequestUpstreamID(mr.ID),
+		properties.PropertyName:                 formatPullRequestName(ns, projName, FormatPullRequestUpstreamID(mr.IID)),
+		properties.PullRequestCommitSHA:         mr.SHA,
+		properties.PullRequestBaseCloneURL:      proj.HTTPURLToRepo,
+		properties.PullRequestBaseDefaultBranch: mr.TargetBranch,
+		properties.PullRequestTargetCloneURL:    targetproj.HTTPURLToRepo,
+		properties.PullRequestTargetBranch:      mr.SourceBranch,
+		properties.PullRequestUpstreamURL:       mr.WebURL,
+		RepoPropertyNamespace:                   ns,
+		RepoPropertyProjectName:                 projName,
 		// internal ID of the merge request
-		PullRequestNumber:       FormatPullRequestUpstreamID(mr.IID),
-		PullRequestProjectID:    FormatRepositoryUpstreamID(proj.ID),
-		PullRequestSourceBranch: mr.SourceBranch,
-		PullRequestTargetBranch: mr.TargetBranch,
-		PullRequestCommitSHA:    mr.SHA,
-		PullRequestAuthor:       int64(mr.Author.ID),
-		PullRequestURL:          mr.WebURL,
+		PullRequestNumber:    FormatPullRequestUpstreamID(mr.IID),
+		PullRequestProjectID: FormatRepositoryUpstreamID(proj.ID),
+		PullRequestAuthor:    int64(mr.Author.ID),
 	})
 	if err != nil {
 		return nil, fmt.Errorf("failed to create properties: %w", err)
@@ -146,12 +158,12 @@ func pullRequestV1FromProperties(prProps *properties.Properties) (*pbinternal.Pu
 		return nil, fmt.Errorf("failed to get project name: %w", err)
 	}
 
-	commitSha, err := getStringProp(prProps, PullRequestCommitSHA)
+	commitSha, err := getStringProp(prProps, properties.PullRequestCommitSHA)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get commit SHA: %w", err)
 	}
 
-	mrURL, err := getStringProp(prProps, PullRequestURL)
+	mrURL, err := getStringProp(prProps, properties.PullRequestUpstreamURL)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get merge request URL: %w", err)
 	}
@@ -161,20 +173,29 @@ func pullRequestV1FromProperties(prProps *properties.Properties) (*pbinternal.Pu
 		return nil, fmt.Errorf("failed to get author ID: %w", err)
 	}
 
+	basecloneurl := prProps.GetProperty(properties.PullRequestBaseCloneURL).GetString()
+	targetcloneurl := prProps.GetProperty(properties.PullRequestTargetCloneURL).GetString()
+	basebranch := prProps.GetProperty(properties.PullRequestBaseDefaultBranch).GetString()
+	targetbranch := prProps.GetProperty(properties.PullRequestTargetBranch).GetString()
+
 	// parse UpstreamID to int64
 	id, err := strconv.ParseInt(iid, 10, 64)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse upstream ID: %w", err)
 	}
 
 	pbPR := &pbinternal.PullRequest{
-		Number:     id,
-		RepoOwner:  ns,
-		RepoName:   projName,
-		CommitSha:  commitSha,
-		AuthorId:   authorID,
-		Url:        mrURL,
-		Properties: prProps.ToProtoStruct(),
+		Number:         id,
+		RepoOwner:      ns,
+		RepoName:       projName,
+		CommitSha:      commitSha,
+		AuthorId:       authorID,
+		Url:            mrURL,
+		BaseCloneUrl:   basecloneurl,
+		TargetCloneUrl: targetcloneurl,
+		BaseRef:        basebranch,
+		TargetRef:      targetbranch,
+		Properties:     prProps.ToProtoStruct(),
 	}
 
 	return pbPR, nil