From ad08eec2a18197d1b1620c59fe5ae91c3ceb2f63 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 10 Dec 2024 12:36:54 +0200 Subject: [PATCH] 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 * Fix target/source confusion Signed-off-by: Juan Antonio Osorio * Enhance logs. Signed-off-by: Juan Antonio Osorio --------- Signed-off-by: Juan Antonio Osorio --- internal/entities/properties/constants.go | 18 +++++ .../github/properties/pull_request.go | 16 ++++- internal/providers/gitlab/properties.go | 8 --- .../gitlab/pull_request_properties.go | 65 ++++++++++++------- 4 files changed, 75 insertions(+), 32 deletions(-) diff --git a/internal/entities/properties/constants.go b/internal/entities/properties/constants.go index d885c3b682..43dc61c276 100644 --- 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 index ffa6803928..e384b85f92 100644 --- 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 index 2b61b30ed7..815812865c 100644 --- 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 index 39c0dde9e3..8ce3364e9d 100644 --- 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,7 +88,15 @@ 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) } @@ -95,7 +104,8 @@ func (c *gitlabClient) getPropertiesForPullRequest( 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,6 +173,11 @@ 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 { @@ -168,13 +185,17 @@ func pullRequestV1FromProperties(prProps *properties.Properties) (*pbinternal.Pu } 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