From ff1c8b346d41bf705e206445643d8a414073b09f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 10 Dec 2024 07:47:15 +0200 Subject: [PATCH 1/3] 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 --- internal/entities/properties/constants.go | 16 +++++ .../github/properties/pull_request.go | 16 ++++- internal/providers/gitlab/properties.go | 8 --- .../gitlab/pull_request_properties.go | 65 ++++++++++++------- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/internal/entities/properties/constants.go b/internal/entities/properties/constants.go index d885c3b682..346fa828b8 100644 --- a/internal/entities/properties/constants.go +++ b/internal/entities/properties/constants.go @@ -21,6 +21,22 @@ 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 + PullRequestTargetCloneURL = "target_clone_url" + // PullRequestTargetBranch represents the default branch of the target repository + 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..33c7ce81c6 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) + sourceproj := proj + if mr.SourceProjectID != 0 && mr.SourceProjectID != proj.ID { + sourceproj, 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, sourceproj) 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, sourceproj *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: sourceproj.HTTPURLToRepo, + properties.PullRequestBaseDefaultBranch: mr.SourceBranch, + properties.PullRequestTargetCloneURL: proj.HTTPURLToRepo, + properties.PullRequestTargetBranch: mr.TargetBranch, + 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 From 96f89c46def22b5a72b424573196139f039aaf32 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 10 Dec 2024 11:43:50 +0200 Subject: [PATCH 2/3] Fix target/source confusion Signed-off-by: Juan Antonio Osorio --- .../providers/gitlab/pull_request_properties.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/providers/gitlab/pull_request_properties.go b/internal/providers/gitlab/pull_request_properties.go index 33c7ce81c6..8ce3364e9d 100644 --- a/internal/providers/gitlab/pull_request_properties.go +++ b/internal/providers/gitlab/pull_request_properties.go @@ -88,15 +88,15 @@ func (c *gitlabClient) getPropertiesForPullRequest( return nil, fmt.Errorf("failed to get project: %w", err) } - sourceproj := proj + targetproj := proj if mr.SourceProjectID != 0 && mr.SourceProjectID != proj.ID { - sourceproj, err = c.getGitLabProject(ctx, FormatRepositoryUpstreamID(mr.SourceProjectID)) + 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, sourceproj) + outProps, err := gitlabMergeRequestToProperties(mr, proj, targetproj) if err != nil { return nil, fmt.Errorf("failed to convert merge request to properties: %w", err) } @@ -105,7 +105,7 @@ func (c *gitlabClient) getPropertiesForPullRequest( } func gitlabMergeRequestToProperties( - mr *gitlab.MergeRequest, proj *gitlab.Project, sourceproj *gitlab.Project) (*properties.Properties, error) { + 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) @@ -118,10 +118,10 @@ func gitlabMergeRequestToProperties( properties.PropertyUpstreamID: FormatPullRequestUpstreamID(mr.ID), properties.PropertyName: formatPullRequestName(ns, projName, FormatPullRequestUpstreamID(mr.IID)), properties.PullRequestCommitSHA: mr.SHA, - properties.PullRequestBaseCloneURL: sourceproj.HTTPURLToRepo, - properties.PullRequestBaseDefaultBranch: mr.SourceBranch, - properties.PullRequestTargetCloneURL: proj.HTTPURLToRepo, - properties.PullRequestTargetBranch: mr.TargetBranch, + properties.PullRequestBaseCloneURL: proj.HTTPURLToRepo, + properties.PullRequestBaseDefaultBranch: mr.TargetBranch, + properties.PullRequestTargetCloneURL: targetproj.HTTPURLToRepo, + properties.PullRequestTargetBranch: mr.SourceBranch, properties.PullRequestUpstreamURL: mr.WebURL, RepoPropertyNamespace: ns, RepoPropertyProjectName: projName, From 0148334527f9ae910689f3d1bbcd9763d532f80f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 10 Dec 2024 11:45:01 +0200 Subject: [PATCH 3/3] Enhance logs. Signed-off-by: Juan Antonio Osorio --- internal/entities/properties/constants.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/entities/properties/constants.go b/internal/entities/properties/constants.go index 346fa828b8..43dc61c276 100644 --- a/internal/entities/properties/constants.go +++ b/internal/entities/properties/constants.go @@ -29,9 +29,11 @@ const ( 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 + // 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 + // 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"