Merge pull request #1013 from tgstation/1012-DeathToNullRefs

Fix posting test merge comments

.github/ISSUE_TEMPLATE.md

@@ -2,10 +2,16 @@ Note that this repository does not contain any client code (With the exception o
 
 Please include:
 
-- A description of the issue.
-
 - The version of tgstation-server you were using.
 
-- Reproduction steps for the issue if possible (Very helpful).
+- A description of the issue.
 
-- Relevent server logs, request logs, and screenshots if possible.
+- A link to your codebase git (if public)
+    - Include active SHA/test merges if applicable
+
+- The client you we're using (Desktop control panel, web control panel, etc)
+    - Include a version if applicable
+
+- Reproduction steps for the issue if possible from your client.
+
+- The server log of when the event happened (The full file is much more useful than snippets).

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,13 +1,19 @@
-[Release Notes]: # (Your PR should contain a detailed list of notable changes, titled appropriately. This includes any observable changes to the server or DMAPI. See example below)
+[Release Notes]: # (Your PR should contain a detailed list of notable changes, titled appropriately. This includes any observable changes to the server or DMAPI. See examples below)
 
 :cl:
-Description of your change
-Each newline corresponds to a release note in the upcoming sprint
+Description of your change.
+Each newline corresponds to a release note in the release your change is included in.
 /:cl:
 
 :cl:
-You can also have multiple sets of release notes per pull request
-They will be amalgamated together in the end
+You can also have multiple sets of release notes per pull request.
+They will be amalgamated together in the end.
 /:cl:
 
-[Why]: # (Please add a short description [two lines down] of why you think these changes would benefit the game. If you can't justify it in words, it might not be worth adding.)
+:cl: Categories
+Categories are divided up in the release notes and set after the :cl: header.
+The default category is Core.
+The current standard categories are Core, DMAPI, HTTP API, and Host Watchdog.
+/:cl:
+
+[Why]: # (If this does not close or work on an existing GitHub issue, please add a short description [two lines down] of why you think these changes would benefit the server. If you can't justify it in words, it might not be worth adding.)

build/Version.props

@@ -2,7 +2,7 @@
   <PropertyGroup>
     <!-- This is the authorative version list -->
     <!-- Integration tests will ensure they match across the board -->
-    <TgsCoreVersion>4.2.4</TgsCoreVersion>
+    <TgsCoreVersion>4.2.5</TgsCoreVersion>
     <TgsApiVersion>6.4.0</TgsApiVersion>
     <TgsClientVersion>6.3.0</TgsClientVersion>
     <TgsDmapiVersion>5.2.1</TgsDmapiVersion>

src/Tgstation.Server.Api/Models/ErrorCode.cs

@@ -483,5 +483,11 @@ namespace Tgstation.Server.Api.Models
 		/// </summary>
 		[Description("Could not bind to requested DreamDaemon port! Is there another service running on that port?")]
 		DreamDaemonPortInUse,
+
+		/// <summary>
+		/// Failed to post GitHub comments, send chat message, or send TGS event.
+		/// </summary>
+		[Description("The deployment succeeded but one or more notification events failed!")]
+		PostDeployFailure,
 	}
 }

src/Tgstation.Server.Host/Components/Deployment/DmbFactory.cs

@@ -196,6 +196,7 @@ namespace Tgstation.Server.Host.Components.Deployment
 				throw new ArgumentNullException(nameof(compileJob));
 
 			// ensure we have the entire compile job tree
+			logger.LogTrace("Loading compile job {0}...", compileJob.Id);
 			await databaseContextFactory.UseContext(async db => compileJob = await db.CompileJobs.Where(x => x.Id == compileJob.Id)
 				.Include(x => x.Job).ThenInclude(x => x.StartedBy)
 				.Include(x => x.RevisionInformation).ThenInclude(x => x.PrimaryTestMerge).ThenInclude(x => x.MergedBy)
@@ -211,7 +212,6 @@ namespace Tgstation.Server.Host.Components.Deployment
 				compileJob.Job.StoppedAt = DateTimeOffset.Now;
 			}
 
-			logger.LogTrace("Loading compile job {0}...", compileJob.Id);
 			var providerSubmitted = false;
 			var newProvider = new DmbProvider(compileJob, ioManager, () =>
 			{

src/Tgstation.Server.Host/Components/Deployment/DreamMaker.cs

@@ -623,7 +623,8 @@ namespace Tgstation.Server.Host.Components.Deployment
 								Instance = new Models.Instance
 								{
 									Id = metadata.Id
-								}
+								},
+								ActiveTestMerges = new List<RevInfoTestMerge>()
 							};
 
 							logger.LogWarning(Repository.Repository.OriginTrackingErrorTemplate, repoSha);
@@ -650,6 +651,7 @@ namespace Tgstation.Server.Host.Components.Deployment
 					cancellationToken)
 					.ConfigureAwait(false);
 
+			var activeCompileJob = compileJobConsumer.LatestCompileJob();
 			try
 			{
 				await databaseContextFactory.UseContext(
@@ -664,12 +666,10 @@ namespace Tgstation.Server.Host.Components.Deployment
 							Id = revInfo.Id
 						};
 
-						databaseContext.RevisionInformations.Attach(compileJob.RevisionInformation);
 						databaseContext.Jobs.Attach(compileJob.Job);
+						databaseContext.RevisionInformations.Attach(compileJob.RevisionInformation);
 						databaseContext.CompileJobs.Add(compileJob);
 
-						await PostDeploymentComments(compileJob, repositorySettings, repoOwner, repoName).ConfigureAwait(false);
-
 						// The difficulty with compile jobs is they have a two part commit
 						await databaseContext.Save(cancellationToken).ConfigureAwait(false);
 						try
@@ -692,15 +692,30 @@ namespace Tgstation.Server.Host.Components.Deployment
 				throw;
 			}
 
-			await eventConsumer.HandleEvent(EventType.DeploymentComplete, null, cancellationToken).ConfigureAwait(false);
+			var commentsTask = PostDeploymentComments(
+				revInfo,
+				activeCompileJob?.RevisionInformation,
+				repositorySettings,
+				repoOwner,
+				repoName);
 
-			await chatManager.SendUpdateMessage(
+			var eventTask = eventConsumer.HandleEvent(EventType.DeploymentComplete, null, cancellationToken);
+
+			var chatTask = chatManager.SendUpdateMessage(
 				String.Format(
 					CultureInfo.InvariantCulture,
 					"Deployment complete! Changes will be applied when DreamDaemon {0}.",
 					watchdog.Running ? "reboots" : "is launched"),
-				cancellationToken)
-				.ConfigureAwait(false);
+				cancellationToken);
+
+			try
+			{
+				await Task.WhenAll(commentsTask, eventTask, chatTask).ConfigureAwait(false);
+			}
+			catch (Exception ex)
+			{
+				throw new JobException(ErrorCode.PostDeployFailure, ex);
+			}
 		}
 		#pragma warning restore CA1506
 
@@ -782,13 +797,15 @@ namespace Tgstation.Server.Host.Components.Deployment
 		/// <summary>
 		/// Post deployment GitHub comments.
 		/// </summary>
-		/// <param name="compileJob">The deployed <see cref="CompileJob"/>.</param>
+		/// <param name="deployedRevisionInformation">The deployed <see cref="RevisionInformation"/>.</param>
+		/// <param name="previousRevisionInformation">The <see cref="RevisionInformation"/> of the previous deployment.</param>
 		/// <param name="repositorySettings">The <see cref="RepositorySettings"/>.</param>
 		/// <param name="repoOwner">The GitHub repostiory owner.</param>
 		/// <param name="repoName">The GitHub repostiory name.</param>
 		/// <returns>A <see cref="Task"/> representing the running operation.</returns>
 		async Task PostDeploymentComments(
-			Models.CompileJob compileJob,
+			Models.RevisionInformation deployedRevisionInformation,
+			Models.RevisionInformation previousRevisionInformation,
 			Models.RepositorySettings repositorySettings,
 			string repoOwner,
 			string repoName)
@@ -796,18 +813,13 @@ namespace Tgstation.Server.Host.Components.Deployment
 			if (repositorySettings?.AccessToken == null)
 				return;
 
-			// potential for commenting on a test merge change
-			var outgoingCompileJob = compileJobConsumer.LatestCompileJob();
-
-			if ((outgoingCompileJob != null && outgoingCompileJob.RevisionInformation.CommitSha == compileJob.RevisionInformation.CommitSha) || !repositorySettings.PostTestMergeComment.Value)
+			if ((previousRevisionInformation != null && previousRevisionInformation.CommitSha == previousRevisionInformation.CommitSha)
+				|| !repositorySettings.PostTestMergeComment.Value)
 				return;
 
-			outgoingCompileJob ??= new Models.CompileJob
+			previousRevisionInformation = new Models.RevisionInformation
 			{
-				RevisionInformation = new Models.RevisionInformation
-				{
-					ActiveTestMerges = new List<RevInfoTestMerge>()
-				}
+				ActiveTestMerges = new List<RevInfoTestMerge>()
 			};
 
 			var gitHubClient = gitHubClientFactory.CreateClient(repositorySettings.AccessToken);
@@ -833,38 +845,32 @@ namespace Tgstation.Server.Host.Components.Deployment
 				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);
+				deployedRevisionInformation.OriginCommitSha,
+				deployedRevisionInformation.CommitSha);
 
 			// added prs
-			foreach (var I in compileJob
-				.RevisionInformation
+			foreach (var I in deployedRevisionInformation
 				.ActiveTestMerges
 				.Select(x => x.TestMerge)
-				.Where(x => !outgoingCompileJob
-					.RevisionInformation
+				.Where(x => !previousRevisionInformation
 					.ActiveTestMerges
 					.Any(y => y.TestMerge.Number == x.Number)))
 				tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, false)));
 
 			// removed prs
-			foreach (var I in outgoingCompileJob
-				.RevisionInformation
+			foreach (var I in previousRevisionInformation
+			.ActiveTestMerges
+			.Select(x => x.TestMerge)
+				.Where(x => !deployedRevisionInformation
 				.ActiveTestMerges
-				.Select(x => x.TestMerge)
-					.Where(x => !compileJob
-					.RevisionInformation
-					.ActiveTestMerges
-					.Any(y => y.TestMerge.Number == x.Number)))
+				.Any(y => y.TestMerge.Number == x.Number)))
 				tasks.Add(CommentOnPR(I.Number, "#### Test Merge Removed"));
 
 			// updated prs
-			foreach (var I in compileJob
-				.RevisionInformation
+			foreach (var I in deployedRevisionInformation
 				.ActiveTestMerges
 				.Select(x => x.TestMerge)
-				.Where(x => outgoingCompileJob
-					.RevisionInformation
+				.Where(x => previousRevisionInformation
 					.ActiveTestMerges
 					.Any(y => y.TestMerge.Number == x.Number)))
 				tasks.Add(CommentOnPR(I.Number, FormatTestMerge(I, true)));

src/Tgstation.Server.Host/Components/README.md

@@ -8,13 +8,15 @@ Component code is where the magic and tears of TGS are made. There are six main
 - [The Watchdog](./Watchdog)
 - [The configuration system](./StaticFiles)
 
-There exist two more namespaces in here that don't fit in these 6.
+There exist two more namespaces in here that don't directly fit in these 6 components.
 
 - [Interop](./Interop) deals with the bulk of DMAPI communication (Though it's not all contained here).
-- [Session](./Session) contains the classes used for actually executing DreamDaemon among other things.
+- [Session](./Session) contains the classes used for actually executing DreamDaemon, sending topic requests, receiving bridge requests, among other things.
 
 Each of these is tied under the roof of an [IInstance](./IInstance.cs) ([implementation](./Instance.cs)).
 
 While the database represents stored instance data, in component code an instance is online, or doesn't exist.
 
 `IInstance`s are created via the [IInstanceFactory](./IInstanceFactory.cs) ([implementation](./InstanceFactory.cs)) and are generally controlled via the [IInstanceManager](./IInstanceManager.cs) ([implementation](./InstanceManager.cs)).
+
+Many classes in here implement [IHostedService](), `InstanceManager` being the only one that is called by the ASP.NET runtime. In the case of instances `StartAsync()` is called when an `Instance` is being brought online (from server startup or user request). The `Instance` handles calling `StartAsync()` on its various subcomponents that need it. When an `Instance` is being brought offline (from server shutdown/restart/update or user request) the same pattern is followed calling `StopAsync()`.

src/Tgstation.Server.Host/Core/Application.cs

@@ -126,9 +126,9 @@ namespace Tgstation.Server.Host.Core
 					var logPath = !String.IsNullOrEmpty(postSetupServices.FileLoggingConfiguration.Directory)
 						? postSetupServices.FileLoggingConfiguration.Directory
 						: IOManager.ConcatPath(
-					Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData),
-					AssemblyInformationProvider.VersionPrefix,
-					"Logs");
+							Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData),
+							AssemblyInformationProvider.VersionPrefix,
+							"Logs");
 
 					var logEventLevel = ConvertSeriLogLevel(postSetupServices.FileLoggingConfiguration.LogLevel);
 
@@ -141,6 +141,7 @@ namespace Tgstation.Server.Host.Core
 						formatter,
 						logPath,
 						logEventLevel ?? LogEventLevel.Verbose,
+						50 * 1024 * 1024, // 50MB max size
 						flushToDiskInterval: TimeSpan.FromSeconds(2));
 				});
 

tests/Tgstation.Server.Tests/IntegrationTest.cs

@@ -115,6 +115,7 @@ namespace Tgstation.Server.Tests
 
 			using var server = new TestingServer();
 			using var serverCts = new CancellationTokenSource();
+			serverCts.CancelAfter(new TimeSpan(0, 9, 30));
 			var cancellationToken = serverCts.Token;
 			TerminateAllDDs();
 			var serverTask = server.Run(cancellationToken);