From cfdc190e5c1535bb001b1e97e7123faf1efeca91 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Wed, 6 May 2020 16:31:39 -0400 Subject: [PATCH] Remove the job post complete step --- .../Components/Instance.cs | 169 ++++++++++-------- src/Tgstation.Server.Host/Jobs/JobManager.cs | 78 +++----- src/Tgstation.Server.Host/Models/Job.cs | 13 +- 3 files changed, 128 insertions(+), 132 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Instance.cs b/src/Tgstation.Server.Host/Components/Instance.cs index b431681550..77975923af 100644 --- a/src/Tgstation.Server.Host/Components/Instance.cs +++ b/src/Tgstation.Server.Host/Components/Instance.cs @@ -261,84 +261,111 @@ namespace Tgstation.Server.Host.Components compileJob.Job = job; - databaseContext.CompileJobs.Add(compileJob); // will be saved by job context + databaseContext.CompileJobs.Add(compileJob); - job.PostComplete = async postCompleteCancellationToken => + // The difficulty with compile jobs is they have a two part commit + await databaseContext.Save(cancellationToken).ConfigureAwait(false); + try { - await compileJobConsumer.LoadCompileJob(compileJob, postCompleteCancellationToken).ConfigureAwait(false); - await eventConsumer.HandleEvent(EventType.DeploymentComplete, null, postCompleteCancellationToken).ConfigureAwait(false); - }; - - if (repositorySettings?.AccessToken != null) + await compileJobConsumer.LoadCompileJob(compileJob, cancellationToken).ConfigureAwait(false); + } + catch { - // potential for commenting on a test merge change - var outgoingCompileJob = LatestCompileJob(); + // So we need to un-commit the compile job if the above throws + databaseContext.CompileJobs.Remove(compileJob); + await databaseContext.Save(default).ConfigureAwait(false); + throw; + } - if (outgoingCompileJob != null && outgoingCompileJob.RevisionInformation.CommitSha != compileJob.RevisionInformation.CommitSha && repositorySettings.PostTestMergeComment.Value) + await eventConsumer.HandleEvent(EventType.DeploymentComplete, null, cancellationToken).ConfigureAwait(false); + + await PostDeploymentComments(compileJob, repositorySettings, repoOwner, repoName).ConfigureAwait(false); + } + + /// + /// Post deployment GitHub comments. + /// + /// The deployed . + /// The . + /// The GitHub repostiory owner. + /// The GitHub repostiory name. + /// A representing the running operation. + async Task PostDeploymentComments( + CompileJob compileJob, + RepositorySettings repositorySettings, + string repoOwner, + string repoName) + { + if (repositorySettings?.AccessToken == null) + return; + + // potential for commenting on a test merge change + var outgoingCompileJob = LatestCompileJob(); + + if (outgoingCompileJob == null || outgoingCompileJob.RevisionInformation.CommitSha == compileJob.RevisionInformation.CommitSha || !repositorySettings.PostTestMergeComment.Value) + return; + + var gitHubClient = gitHubClientFactory.CreateClient(repositorySettings.AccessToken); + + async Task CommentOnPR(int prNumber, string comment) + { + try { - var gitHubClient = gitHubClientFactory.CreateClient(repositorySettings.AccessToken); - - async Task CommentOnPR(int prNumber, string comment) - { - try - { - await gitHubClient.Issue.Comment.Create(repoOwner, repoName, prNumber, comment).ConfigureAwait(false); - } - catch (ApiException e) - { - logger.LogWarning("Error posting GitHub comment! Exception: {0}", e); - } - } - - var tasks = new List(); - - string FormatTestMerge(TestMerge testMerge, bool updated) => String.Format(CultureInfo.InvariantCulture, "#### Test Merge {4}{0}{0}##### Server Instance{0}{5}{1}{0}{0}##### Revision{0}Origin: {6}{0}Pull Request: {2}{0}Server: {7}{3}", - Environment.NewLine, - repositorySettings.ShowTestMergeCommitters.Value ? String.Format(CultureInfo.InvariantCulture, "{0}{0}##### Merged By{0}{1}", Environment.NewLine, testMerge.MergedBy.Name) : String.Empty, - testMerge.PullRequestRevision, - testMerge.Comment != null ? String.Format(CultureInfo.InvariantCulture, "{0}{0}##### Comment{0}{1}", Environment.NewLine, testMerge.Comment) : String.Empty, - updated ? "Updated" : "Deployed", - metadata.Name, - compileJob.RevisionInformation.OriginCommitSha, - compileJob.RevisionInformation.CommitSha); - - // added prs - foreach (var I in compileJob - .RevisionInformation - .ActiveTestMerges - .Select(x => x.TestMerge) - .Where(x => !outgoingCompileJob - .RevisionInformation - .ActiveTestMerges - .Any(y => y.TestMerge.Number == x.Number))) - tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, false))); - - // removed prs - foreach (var I in outgoingCompileJob - .RevisionInformation - .ActiveTestMerges - .Select(x => x.TestMerge) - .Where(x => !compileJob - .RevisionInformation - .ActiveTestMerges - .Any(y => y.TestMerge.Number == x.Number))) - tasks.Add(CommentOnPR(I.Number, "#### Test Merge Removed")); - - // updated prs - foreach (var I in compileJob - .RevisionInformation - .ActiveTestMerges - .Select(x => x.TestMerge) - .Where(x => outgoingCompileJob - .RevisionInformation - .ActiveTestMerges - .Any(y => y.TestMerge.Number == x.Number))) - tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, true))); - - if (tasks.Any()) - await Task.WhenAll(tasks).ConfigureAwait(false); + await gitHubClient.Issue.Comment.Create(repoOwner, repoName, prNumber, comment).ConfigureAwait(false); + } + catch (ApiException e) + { + logger.LogWarning("Error posting GitHub comment! Exception: {0}", e); } } + + var tasks = new List(); + + string FormatTestMerge(TestMerge testMerge, bool updated) => String.Format(CultureInfo.InvariantCulture, "#### Test Merge {4}{0}{0}##### Server Instance{0}{5}{1}{0}{0}##### Revision{0}Origin: {6}{0}Pull Request: {2}{0}Server: {7}{3}", + Environment.NewLine, + repositorySettings.ShowTestMergeCommitters.Value ? String.Format(CultureInfo.InvariantCulture, "{0}{0}##### Merged By{0}{1}", Environment.NewLine, testMerge.MergedBy.Name) : String.Empty, + testMerge.PullRequestRevision, + testMerge.Comment != null ? String.Format(CultureInfo.InvariantCulture, "{0}{0}##### Comment{0}{1}", Environment.NewLine, testMerge.Comment) : String.Empty, + updated ? "Updated" : "Deployed", + metadata.Name, + compileJob.RevisionInformation.OriginCommitSha, + compileJob.RevisionInformation.CommitSha); + + // added prs + foreach (var I in compileJob + .RevisionInformation + .ActiveTestMerges + .Select(x => x.TestMerge) + .Where(x => !outgoingCompileJob + .RevisionInformation + .ActiveTestMerges + .Any(y => y.TestMerge.Number == x.Number))) + tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, false))); + + // removed prs + foreach (var I in outgoingCompileJob + .RevisionInformation + .ActiveTestMerges + .Select(x => x.TestMerge) + .Where(x => !compileJob + .RevisionInformation + .ActiveTestMerges + .Any(y => y.TestMerge.Number == x.Number))) + tasks.Add(CommentOnPR(I.Number, "#### Test Merge Removed")); + + // updated prs + foreach (var I in compileJob + .RevisionInformation + .ActiveTestMerges + .Select(x => x.TestMerge) + .Where(x => outgoingCompileJob + .RevisionInformation + .ActiveTestMerges + .Any(y => y.TestMerge.Number == x.Number))) + tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, true))); + + if (tasks.Any()) + await Task.WhenAll(tasks).ConfigureAwait(false); } /// diff --git a/src/Tgstation.Server.Host/Jobs/JobManager.cs b/src/Tgstation.Server.Host/Jobs/JobManager.cs index a3919c8dd2..20e97c3e1d 100644 --- a/src/Tgstation.Server.Host/Jobs/JobManager.cs +++ b/src/Tgstation.Server.Host/Jobs/JobManager.cs @@ -81,43 +81,8 @@ namespace Tgstation.Server.Host.Jobs { await databaseContextFactory.UseContext(async databaseContext => { - async Task HandleExceptions(Task task) - { - void LogRegularException() => logger.LogDebug("Job {0} exited with error! Exception: {1}", job.Id, job.ExceptionDetails); - try - { - await task.ConfigureAwait(false); - } - catch (OperationCanceledException) - { - logger.LogDebug("Job {0} cancelled!", job.Id); - job.Cancelled = true; - } - catch (JobException e) - { - job.ErrorCode = e.ErrorCode; - job.ExceptionDetails = e.Message; - LogRegularException(); - if (e.InnerException != null) - logger.LogDebug( - "Inner exception for job {0}: {1}", - job.Id, - e.InnerException is JobException - ? e.InnerException.Message - : e.InnerException.ToString()); - } - catch (Exception e) - { - job.ExceptionDetails = e.ToString(); - LogRegularException(); - } - finally - { - job.StoppedAt = DateTimeOffset.Now; - } - } - - async Task RunJobInternal() + void LogRegularException() => logger.LogDebug("Job {0} exited with error! Exception: {1}", job.Id, job.ExceptionDetails); + try { var oldJob = job; job = new Job { Id = oldJob.Id }; @@ -127,20 +92,35 @@ namespace Tgstation.Server.Host.Jobs logger.LogDebug("Job {0} completed!", job.Id); } - - await HandleExceptions(RunJobInternal()).ConfigureAwait(false); + catch (OperationCanceledException) + { + logger.LogDebug("Job {0} cancelled!", job.Id); + job.Cancelled = true; + } + catch (JobException e) + { + job.ErrorCode = e.ErrorCode; + job.ExceptionDetails = e.Message; + LogRegularException(); + if (e.InnerException != null) + logger.LogDebug( + "Inner exception for job {0}: {1}", + job.Id, + e.InnerException is JobException + ? e.InnerException.Message + : e.InnerException.ToString()); + } + catch (Exception e) + { + job.ExceptionDetails = e.ToString(); + LogRegularException(); + } + finally + { + job.StoppedAt = DateTimeOffset.Now; + } await databaseContext.Save(default).ConfigureAwait(false); - - bool JobErroredOrCancelled() => job.ExceptionDetails != null || job.Cancelled == true; - - // ok so, now it's time for the post commit step if it exists - if (!JobErroredOrCancelled() && job.PostComplete != null) - { - await HandleExceptions(job.PostComplete(cancellationToken)).ConfigureAwait(false); - if (JobErroredOrCancelled()) - await databaseContext.Save(default).ConfigureAwait(false); - } }).ConfigureAwait(false); } finally diff --git a/src/Tgstation.Server.Host/Models/Job.cs b/src/Tgstation.Server.Host/Models/Job.cs index 7ff111762c..134993f14c 100644 --- a/src/Tgstation.Server.Host/Models/Job.cs +++ b/src/Tgstation.Server.Host/Models/Job.cs @@ -1,8 +1,4 @@ -using System; -using System.ComponentModel.DataAnnotations; -using System.ComponentModel.DataAnnotations.Schema; -using System.Threading; -using System.Threading.Tasks; +using System.ComponentModel.DataAnnotations; namespace Tgstation.Server.Host.Models { @@ -26,13 +22,6 @@ namespace Tgstation.Server.Host.Models [Required] public Instance Instance { get; set; } - /// - /// A to run after the job completes. This will not affect the time, unless it is cancelled or errors - /// - /// This should only be used where there are database dependencies that also rely on the Job itself completing A.K.A. manually initiated s - [NotMapped] - public Func PostComplete { get; set; } - /// /// Convert the to it's API form ///