From 50b66d9342cb59fa69cf766842c2f278853b7ddf Mon Sep 17 00:00:00 2001 From: Johannes Batzill Date: Tue, 12 Sep 2023 16:26:18 +0000 Subject: [PATCH] [PR] Recheck `Unchecked` PR Merge Status (#434) --- cmd/gitness/wire_gen.go | 24 ++++----- internal/api/controller/pullreq/controller.go | 4 ++ internal/api/controller/pullreq/pr_recheck.go | 33 ++++++++++++ internal/api/controller/pullreq/wire.go | 7 ++- internal/api/handler/pullreq/pr_recheck.go | 41 ++++++++++++++ internal/api/openapi/pullreq.go | 11 ++++ internal/router/api.go | 1 + .../services/pullreq/handlers_mergeable.go | 53 ++++++++++++++++--- web/src/pages/PullRequest/PullRequest.tsx | 33 +++++++++++- web/src/services/code/index.tsx | 34 ++++++++++++ web/src/services/code/swagger.yaml | 48 +++++++++++++++++ 11 files changed, 266 insertions(+), 23 deletions(-) create mode 100644 internal/api/controller/pullreq/pr_recheck.go create mode 100644 internal/api/handler/pullreq/pr_recheck.go diff --git a/cmd/gitness/wire_gen.go b/cmd/gitness/wire_gen.go index 39121cf0b..abf0eb21f 100644 --- a/cmd/gitness/wire_gen.go +++ b/cmd/gitness/wire_gen.go @@ -22,7 +22,7 @@ import ( "github.com/harness/gitness/internal/api/controller/pipeline" "github.com/harness/gitness/internal/api/controller/plugin" "github.com/harness/gitness/internal/api/controller/principal" - "github.com/harness/gitness/internal/api/controller/pullreq" + pullreq2 "github.com/harness/gitness/internal/api/controller/pullreq" "github.com/harness/gitness/internal/api/controller/repo" "github.com/harness/gitness/internal/api/controller/secret" "github.com/harness/gitness/internal/api/controller/service" @@ -51,7 +51,7 @@ import ( "github.com/harness/gitness/internal/services/codecomments" "github.com/harness/gitness/internal/services/importer" "github.com/harness/gitness/internal/services/job" - pullreq2 "github.com/harness/gitness/internal/services/pullreq" + "github.com/harness/gitness/internal/services/pullreq" trigger2 "github.com/harness/gitness/internal/services/trigger" "github.com/harness/gitness/internal/services/webhook" "github.com/harness/gitness/internal/store" @@ -174,10 +174,6 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro return nil, err } migrator := codecomments.ProvideMigrator(gitrpcInterface) - pullreqController := pullreq.ProvideController(db, provider, authorizer, pullReqStore, pullReqActivityStore, codeCommentView, pullReqReviewStore, pullReqReviewerStore, repoStore, principalStore, gitrpcInterface, reporter, mutexManager, migrator) - webhookConfig := server.ProvideWebhookConfig(config) - webhookStore := database.ProvideWebhookStore(db) - webhookExecutionStore := database.ProvideWebhookExecutionStore(db) readerFactory, err := events4.ProvideReaderFactory(eventsSystem) if err != nil { return nil, err @@ -186,6 +182,16 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro if err != nil { return nil, err } + repoGitInfoView := database.ProvideRepoGitInfoView(db) + repoGitInfoCache := cache.ProvideRepoGitInfoCache(repoGitInfoView) + pullreqService, err := pullreq.ProvideService(ctx, config, readerFactory, eventsReaderFactory, reporter, gitrpcInterface, db, repoGitInfoCache, repoStore, pullReqStore, pullReqActivityStore, codeCommentView, migrator, pubSub, provider) + if err != nil { + return nil, err + } + pullreqController := pullreq2.ProvideController(db, provider, authorizer, pullReqStore, pullReqActivityStore, codeCommentView, pullReqReviewStore, pullReqReviewerStore, repoStore, principalStore, gitrpcInterface, reporter, mutexManager, migrator, pullreqService) + webhookConfig := server.ProvideWebhookConfig(config) + webhookStore := database.ProvideWebhookStore(db) + webhookExecutionStore := database.ProvideWebhookExecutionStore(db) webhookService, err := webhook.ProvideService(ctx, webhookConfig, readerFactory, eventsReaderFactory, webhookStore, webhookExecutionStore, repoStore, pullReqStore, provider, principalStore, gitrpcInterface) if err != nil { return nil, err @@ -228,12 +234,6 @@ func initSystem(ctx context.Context, config *types.Config) (*server.System, erro return nil, err } cronManager := cron.ProvideManager(serverConfig) - repoGitInfoView := database.ProvideRepoGitInfoView(db) - repoGitInfoCache := cache.ProvideRepoGitInfoCache(repoGitInfoView) - pullreqService, err := pullreq2.ProvideService(ctx, config, readerFactory, eventsReaderFactory, reporter, gitrpcInterface, db, repoGitInfoCache, repoStore, pullReqStore, pullReqActivityStore, codeCommentView, migrator, pubSub, provider) - if err != nil { - return nil, err - } triggerConfig := server.ProvideTriggerConfig(config) triggerService, err := trigger2.ProvideService(ctx, triggerConfig, triggerStore, commitService, pullReqStore, repoStore, pipelineStore, triggererTriggerer, readerFactory, eventsReaderFactory) if err != nil { diff --git a/internal/api/controller/pullreq/controller.go b/internal/api/controller/pullreq/controller.go index e96c9c761..d7baa809b 100644 --- a/internal/api/controller/pullreq/controller.go +++ b/internal/api/controller/pullreq/controller.go @@ -16,6 +16,7 @@ import ( "github.com/harness/gitness/internal/auth/authz" pullreqevents "github.com/harness/gitness/internal/events/pullreq" "github.com/harness/gitness/internal/services/codecomments" + "github.com/harness/gitness/internal/services/pullreq" "github.com/harness/gitness/internal/store" "github.com/harness/gitness/internal/url" "github.com/harness/gitness/lock" @@ -40,6 +41,7 @@ type Controller struct { eventReporter *pullreqevents.Reporter mtxManager lock.MutexManager codeCommentMigrator *codecomments.Migrator + pullreqService *pullreq.Service } func NewController( @@ -57,6 +59,7 @@ func NewController( eventReporter *pullreqevents.Reporter, mtxManager lock.MutexManager, codeCommentMigrator *codecomments.Migrator, + pullreqService *pullreq.Service, ) *Controller { return &Controller{ db: db, @@ -73,6 +76,7 @@ func NewController( codeCommentMigrator: codeCommentMigrator, eventReporter: eventReporter, mtxManager: mtxManager, + pullreqService: pullreqService, } } diff --git a/internal/api/controller/pullreq/pr_recheck.go b/internal/api/controller/pullreq/pr_recheck.go new file mode 100644 index 000000000..f35848810 --- /dev/null +++ b/internal/api/controller/pullreq/pr_recheck.go @@ -0,0 +1,33 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "context" + "fmt" + + "github.com/harness/gitness/internal/auth" + "github.com/harness/gitness/types/enum" +) + +// Recheck re-checks all system PR checks (mergeability check, ...). +func (c *Controller) Recheck( + ctx context.Context, + session *auth.Session, + repoRef string, + prNum int64, +) error { + repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush) + if err != nil { + return fmt.Errorf("failed to acquire access to repo: %w", err) + } + + err = c.pullreqService.UpdateMergeDataIfRequired(ctx, repo.ID, prNum) + if err != nil { + return fmt.Errorf("failed to refresh merge data: %w", err) + } + + return nil +} diff --git a/internal/api/controller/pullreq/wire.go b/internal/api/controller/pullreq/wire.go index 8f6c3a757..e313c49bd 100644 --- a/internal/api/controller/pullreq/wire.go +++ b/internal/api/controller/pullreq/wire.go @@ -9,6 +9,7 @@ import ( "github.com/harness/gitness/internal/auth/authz" pullreqevents "github.com/harness/gitness/internal/events/pullreq" "github.com/harness/gitness/internal/services/codecomments" + "github.com/harness/gitness/internal/services/pullreq" "github.com/harness/gitness/internal/store" "github.com/harness/gitness/internal/url" "github.com/harness/gitness/lock" @@ -28,12 +29,14 @@ func ProvideController(db *sqlx.DB, urlProvider *url.Provider, authorizer authz. pullReqReviewStore store.PullReqReviewStore, pullReqReviewerStore store.PullReqReviewerStore, repoStore store.RepoStore, principalStore store.PrincipalStore, rpcClient gitrpc.Interface, eventReporter *pullreqevents.Reporter, - mtxManager lock.MutexManager, codeCommentMigrator *codecomments.Migrator) *Controller { + mtxManager lock.MutexManager, codeCommentMigrator *codecomments.Migrator, + pullreqService *pullreq.Service, +) *Controller { return NewController(db, urlProvider, authorizer, pullReqStore, pullReqActivityStore, codeCommentsView, pullReqReviewStore, pullReqReviewerStore, repoStore, principalStore, rpcClient, eventReporter, - mtxManager, codeCommentMigrator) + mtxManager, codeCommentMigrator, pullreqService) } diff --git a/internal/api/handler/pullreq/pr_recheck.go b/internal/api/handler/pullreq/pr_recheck.go new file mode 100644 index 000000000..212e37f12 --- /dev/null +++ b/internal/api/handler/pullreq/pr_recheck.go @@ -0,0 +1,41 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "net/http" + + "github.com/harness/gitness/internal/api/controller/pullreq" + "github.com/harness/gitness/internal/api/render" + "github.com/harness/gitness/internal/api/request" +) + +// HandleRecheck handles API that re-checks all system PR checks (mergeability check, ...). +func HandleRecheck(pullreqCtrl *pullreq.Controller) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + session, _ := request.AuthSessionFrom(ctx) + + repoRef, err := request.GetRepoRefFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + pullreqNumber, err := request.GetPullReqNumberFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + err = pullreqCtrl.Recheck(ctx, session, repoRef, pullreqNumber) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + w.WriteHeader(http.StatusNoContent) + } +} diff --git a/internal/api/openapi/pullreq.go b/internal/api/openapi/pullreq.go index fdff3d37f..693da8daa 100644 --- a/internal/api/openapi/pullreq.go +++ b/internal/api/openapi/pullreq.go @@ -469,4 +469,15 @@ func pullReqOperations(reflector *openapi3.Reflector) { _ = reflector.SetJSONResponse(&opMetaData, new(usererror.Error), http.StatusForbidden) _ = reflector.SetJSONResponse(&opMetaData, new(usererror.Error), http.StatusNotFound) _ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/pullreq/{pullreq_number}/metadata", opMetaData) + + recheckPullReq := openapi3.Operation{} + recheckPullReq.WithTags("pullreq") + recheckPullReq.WithMapOfAnything(map[string]interface{}{"operationId": "recheckPullReq"}) + _ = reflector.SetRequest(&recheckPullReq, nil, http.MethodPost) + _ = reflector.SetJSONResponse(&recheckPullReq, nil, http.StatusNoContent) + _ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusBadRequest) + _ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusInternalServerError) + _ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusUnauthorized) + _ = reflector.SetJSONResponse(&recheckPullReq, new(usererror.Error), http.StatusForbidden) + _ = reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/pullreq/{pullreq_number}/recheck", recheckPullReq) } diff --git a/internal/router/api.go b/internal/router/api.go index d2fa4762f..6b1b57f54 100644 --- a/internal/router/api.go +++ b/internal/router/api.go @@ -461,6 +461,7 @@ func SetupPullReq(r chi.Router, pullreqCtrl *pullreq.Controller) { r.Get("/", handlerpullreq.HandleFind(pullreqCtrl)) r.Patch("/", handlerpullreq.HandleUpdate(pullreqCtrl)) r.Post("/state", handlerpullreq.HandleState(pullreqCtrl)) + r.Post("/recheck", handlerpullreq.HandleRecheck(pullreqCtrl)) r.Get("/activities", handlerpullreq.HandleListActivities(pullreqCtrl)) r.Route("/comments", func(r chi.Router) { r.Post("/", handlerpullreq.HandleCommentCreate(pullreqCtrl)) diff --git a/internal/services/pullreq/handlers_mergeable.go b/internal/services/pullreq/handlers_mergeable.go index 4fe14c6ca..54262e350 100644 --- a/internal/services/pullreq/handlers_mergeable.go +++ b/internal/services/pullreq/handlers_mergeable.go @@ -106,6 +106,28 @@ func (s *Service) deleteMergeRef(ctx context.Context, repoID int64, prNum int64) return nil } +// UpdateMergeDataIfRequired rechecks the merge data of a PR. +// TODO: This is a temporary solution - doesn't fix changed merge-base or other things. +func (s *Service) UpdateMergeDataIfRequired( + ctx context.Context, + repoID int64, + prNum int64, +) error { + pr, err := s.pullreqStore.FindByNumber(ctx, repoID, prNum) + if err != nil { + return fmt.Errorf("failed to get pull request number %d: %w", prNum, err) + } + + // nothing to-do if check was already performed + if pr.MergeCheckStatus != enum.MergeCheckStatusUnchecked { + return nil + } + + // WARNING: This CAN lead to two (or more) merge-checks on the same SHA + // running on different machines at the same time. + return s.updateMergeDataInner(ctx, pr, "", pr.SourceSHA) +} + //nolint:funlen // refactor if required. func (s *Service) updateMergeData( ctx context.Context, @@ -114,21 +136,31 @@ func (s *Service) updateMergeData( oldSHA string, newSHA string, ) error { - // TODO: Merge check should not update the merge base. - // TODO: Instead it should accept it as an argument and fail if it doesn't match. - // Then is would not longer be necessary to cancel already active mergeability checks. - pr, err := s.pullreqStore.FindByNumber(ctx, repoID, prNum) if err != nil { return fmt.Errorf("failed to get pull request number %d: %w", prNum, err) } + return s.updateMergeDataInner(ctx, pr, oldSHA, newSHA) +} + +//nolint:funlen // refactor if required. +func (s *Service) updateMergeDataInner( + ctx context.Context, + pr *types.PullReq, + oldSHA string, + newSHA string, +) error { + // TODO: Merge check should not update the merge base. + // TODO: Instead it should accept it as an argument and fail if it doesn't match. + // Then is would not longer be necessary to cancel already active mergeability checks. + if pr.State != enum.PullReqStateOpen { - return fmt.Errorf("cannot do mergability check on closed PR %d", prNum) + return fmt.Errorf("cannot do mergability check on closed PR %d", pr.Number) } // cancel all previous mergability work for this PR based on oldSHA - if err = s.pubsub.Publish(ctx, cancelMergeCheckKey, []byte(oldSHA), + if err := s.pubsub.Publish(ctx, cancelMergeCheckKey, []byte(oldSHA), pubsub.WithPublishNamespace("pullreq")); err != nil { return err } @@ -137,6 +169,13 @@ func (s *Service) updateMergeData( ctx, cancel = context.WithCancel(ctx) s.cancelMutex.Lock() + // NOTE: Temporary workaround to avoid overwriting existing cancel method on same machine. + // This doesn't avoid same SHA running on multiple machines + if _, ok := s.cancelMergeability[newSHA]; ok { + s.cancelMutex.Unlock() + cancel() + return nil + } s.cancelMergeability[newSHA] = cancel s.cancelMutex.Unlock() @@ -175,7 +214,7 @@ func (s *Service) updateMergeData( HeadRepoUID: sourceRepo.GitUID, HeadBranch: pr.SourceBranch, RefType: gitrpcenum.RefTypePullReqMerge, - RefName: strconv.Itoa(int(prNum)), + RefName: strconv.Itoa(int(pr.Number)), HeadExpectedSHA: newSHA, Force: true, diff --git a/web/src/pages/PullRequest/PullRequest.tsx b/web/src/pages/PullRequest/PullRequest.tsx index 9659a1936..a5721b557 100644 --- a/web/src/pages/PullRequest/PullRequest.tsx +++ b/web/src/pages/PullRequest/PullRequest.tsx @@ -1,7 +1,7 @@ import React, { useCallback, useEffect, useMemo, useState } from 'react' import { Container, Layout, PageBody, Tabs, Text } from '@harnessio/uicore' import { FontVariation } from '@harnessio/design-system' -import { useGet } from 'restful-react' +import { useGet, useMutate } from 'restful-react' import { Render } from 'react-jsx-match' import { useHistory } from 'react-router-dom' import { compact } from 'lodash-es' @@ -9,7 +9,7 @@ import { useAppContext } from 'AppContext' import { useGetRepositoryMetadata } from 'hooks/useGetRepositoryMetadata' import { useStrings } from 'framework/strings' import { RepositoryPageHeader } from 'components/RepositoryPageHeader/RepositoryPageHeader' -import { voidFn, getErrorMessage, PullRequestSection } from 'utils/Utils' +import { voidFn, getErrorMessage, PullRequestSection, MergeCheckStatus } from 'utils/Utils' import { CodeIcon } from 'utils/GitUtils' import type { TypesPullReq, TypesPullReqStats, TypesRepository } from 'services/code' import { LoadingSpinner } from 'components/LoadingSpinner/LoadingSpinner' @@ -83,6 +83,14 @@ export default function PullRequest() { }) ) }, [history, routes, repoMetadata?.path, pullRequestId]) + const recheckPath = useMemo( + () => `/api/v1/repos/${repoMetadata?.path}/+/pullreq/${pullRequestId}/recheck`, + [repoMetadata?.path, pullRequestId] + ) + const { mutate : recheckPR, loading: loadingRecheckPR } = useMutate({ + verb: 'POST', + path: recheckPath, + }) useEffect( function setStatsIfNotSet() { @@ -98,6 +106,27 @@ export default function PullRequest() { useEffect( function setPrDataIfNotSet() { if (pullRequestData) { + // recheck pr (merge-check, ...) in case it's unavailable + // Approximation of identifying target branch update: + // 1. branch got updated before page was loaded (status is unchecked and prData is empty) + // NOTE: This doesn't guarantee the status is UNCHECKED due to target branch update and can cause duplicate + // PR merge checks being run on PR creation or source branch update. + // 2. branch got updated while we are on the page (same source_sha but status changed to UNCHECKED) + // NOTE: This doesn't cover the case in which the status changed back to UNCHECKED before the PR is refetched. + // In that case, the user will have to re-open the PR - better than us spamming the backend with rechecks. + // This is a TEMPORARY SOLUTION and will most likely change in the future (more so on backend side) + if (pullRequestData.state == 'open' && + pullRequestData.merge_check_status == MergeCheckStatus.UNCHECKED && + ( + // case 1: + !prData || + // case 2: + (prData?.merge_check_status != MergeCheckStatus.UNCHECKED && prData?.source_sha == pullRequestData.source_sha) + ) && !loadingRecheckPR) { + // best effort attempt to recheck PR - fail silently + recheckPR({}) + } + setPrData(pullRequestData) } }, diff --git a/web/src/services/code/index.tsx b/web/src/services/code/index.tsx index 3779663e7..c50135836 100644 --- a/web/src/services/code/index.tsx +++ b/web/src/services/code/index.tsx @@ -3398,6 +3398,40 @@ export const usePullReqMetaData = ({ repo_ref, pullreq_number, ...props }: UsePu { base: getConfig('code/api/v1'), pathParams: { repo_ref, pullreq_number }, ...props } ) +export interface RecheckPullReqPathParams { + repo_ref: string + pullreq_number: number +} + +export type RecheckPullReqProps = Omit< + MutateProps, + 'path' | 'verb' +> & + RecheckPullReqPathParams + +export const RecheckPullReq = ({ repo_ref, pullreq_number, ...props }: RecheckPullReqProps) => ( + + verb="POST" + path={`/repos/${repo_ref}/pullreq/${pullreq_number}/recheck`} + base={getConfig('code/api/v1')} + {...props} + /> +) + +export type UseRecheckPullReqProps = Omit< + UseMutateProps, + 'path' | 'verb' +> & + RecheckPullReqPathParams + +export const useRecheckPullReq = ({ repo_ref, pullreq_number, ...props }: UseRecheckPullReqProps) => + useMutate( + 'POST', + (paramsInPath: RecheckPullReqPathParams) => + `/repos/${paramsInPath.repo_ref}/pullreq/${paramsInPath.pullreq_number}/recheck`, + { base: getConfig('code/api/v1'), pathParams: { repo_ref, pullreq_number }, ...props } + ) + export interface ReviewerListPullReqPathParams { repo_ref: string pullreq_number: number diff --git a/web/src/services/code/swagger.yaml b/web/src/services/code/swagger.yaml index 27a2a1659..14f78da2e 100644 --- a/web/src/services/code/swagger.yaml +++ b/web/src/services/code/swagger.yaml @@ -3622,6 +3622,54 @@ paths: description: Internal Server Error tags: - pullreq + /repos/{repo_ref}/pullreq/{pullreq_number}/recheck: + post: + operationId: recheckPullReq + parameters: + - in: path + name: repo_ref + required: true + schema: + type: string + - in: path + name: pullreq_number + required: true + schema: + type: integer + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/OpenapiUpdatePullReqRequest' + responses: + "204": + description: No Content + "400": + content: + application/json: + schema: + $ref: '#/components/schemas/UsererrorError' + description: Bad Request + "401": + content: + application/json: + schema: + $ref: '#/components/schemas/UsererrorError' + description: Unauthorized + "403": + content: + application/json: + schema: + $ref: '#/components/schemas/UsererrorError' + description: Forbidden + "500": + content: + application/json: + schema: + $ref: '#/components/schemas/UsererrorError' + description: Internal Server Error + tags: + - pullreq /repos/{repo_ref}/pullreq/{pullreq_number}/reviewers: get: operationId: reviewerListPullReq