Helix SDK Avoids Upload for Disabled Queues #4011

Tohron · 2019-09-25T20:57:49Z

This fix detects if a specified target queue has IsAvailable = false, and if so, does not upload, and logs either an error or a warning (depending on whether ProceedIfQueueDisabled is true, or the default false)

dnfclas · 2019-09-25T20:58:09Z

All CLA requirements met.

Tohron · 2019-09-25T23:15:05Z

Fix for issue 3892: #3892

Tohron · 2019-09-26T20:54:36Z

Right now, I'm trying to determine what the exact objectives are for what the PR should achieve.

MattGal · 2019-10-01T17:15:35Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

@@ -125,6 +126,11 @@ public class SendHelixJob : HelixTask
        /// </summary>
        public int MaxRetryCount { get; set; }

+        /// <summary>
+        ///   <see langword="true"/> when a warning will be thrown when a queue is disabled,; <see langword="false"/> (default value) when an error will be thrown.


Poor readability, please reword.

Suggestion: "When True, issue a warning when a queue is unavailable; when False, issue an error to fail the build"

MattGal · 2019-10-01T17:16:14Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

@@ -149,6 +155,22 @@ protected override async Task ExecuteCore(CancellationToken cancellationToken)
            {
                var currentHelixApi = HelixApi;

+                Client.Models.QueueInfo qi = await currentHelixApi.Information.QueueInfoAsync(TargetQueue);
+                bool? availability = qi.IsAvailable;


Can definitely combine lines 159 and 160

MattGal · 2019-10-01T17:18:58Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

+        /// <summary>
+        ///   <see langword="true"/> when a warning will be thrown when a queue is disabled,; <see langword="false"/> (default value) when an error will be thrown.
+        /// </summary>
+        public bool ProceedIfQueueDisabled { get; set; } = false;


In neither cases do we proceed. Can this be named something that indicates what it does, e.g. `ErrorOnUnavailableQueues" or something?

To clarify, is the behavior when it only throws a warning that (after skipping uploading to the current queue) it continues with whatever it was doing?

no you return from the task (which I agree with). That's my objection to the term "proceed"

MattGal

Functionally seems like the right fix, please clean up the property naming and comments.

alexperovich · 2019-10-01T17:24:01Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

@@ -149,6 +155,22 @@ protected override async Task ExecuteCore(CancellationToken cancellationToken)
            {
                var currentHelixApi = HelixApi;

+                Client.Models.QueueInfo qi = await currentHelixApi.Information.QueueInfoAsync(TargetQueue);


This Client.Models isn't necessary, just add the appropriate using.

alexperovich · 2019-10-01T17:26:08Z

src/Microsoft.DotNet.Helix/Sdk/tools/Microsoft.DotNet.Helix.Sdk.MonoQueue.targets

@@ -55,7 +56,8 @@
                  PostCommands="$(HelixPostCommands)"
                  CorrelationPayloads="@(HelixCorrelationPayload)"
                  WorkItems="@(HelixWorkItem)"
-                  HelixProperties="@(HelixProperties)">
+                  HelixProperties="@(HelixProperties)"
+				  ProceedIfQueueDisabled="$(ProceedIfDisabled)">


Fix formatting please

MattGal · 2019-10-01T17:27:54Z

tests/UnitTests.proj

@@ -73,6 +73,7 @@
    <HelixTargetQueue Include="Debian.9.Amd64.Open"/>
    <HelixTargetQueue Include="RedHat.7.Amd64.Open"/>
    <HelixTargetQueue Include="Windows.10.Amd64.Open"/>
+    <HelixTargetQueue Include="Alpine.36.Amd64"/>


Can't have this here if you ever want your build to be green

Perhaps I could leave it commented-out, with another comment describing what it does?

MattGal · 2019-10-01T17:28:01Z

tests/UnitTests.proj

@@ -12,7 +12,7 @@
    <IncludeDotNetCli>true</IncludeDotNetCli>
    <DotNetCliPackageType>sdk</DotNetCliPackageType>

-    <EnableAzurePipelinesReporter>true</EnableAzurePipelinesReporter>
+    <EnableAzurePipelinesReporter>false</EnableAzurePipelinesReporter>


I don't remember if this change was made for debugging purposes. I suppose I should revert it?

Tohron · 2019-10-01T21:06:01Z

src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs

@@ -23,7 +23,7 @@ namespace Microsoft.Cci.Extensions.CSharp
    public static class CSharpCciExtensions


This file somehow got added back in after I removed it (it was originally added when I resolved a merge conflict the wrong way).

MattGal · 2019-10-01T23:10:56Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

        /// </summary>
-        public bool ProceedIfQueueDisabled { get; set; } = false;
+        public bool NoErrorIfQueueDisabled { get; set; } = false;


sorry to be so nitpicky. This variable is a boolean. The No or Yes of Error If queue is disabled is covered by the true/falseness of the variable.

It makes a lot more sense for Error to be false versus NoError to be False meaning error.

MattGal

LGTM, consider the one little bit of cleanup but not blocking.

MattGal · 2019-10-03T20:09:26Z

src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs

+                {
+                    if (LogErrorIfQueueDisabled)
+                    {
+                        Log.LogError("Queue is not available");


nit: Doesn't hurt to log the queue name here, e.g. Log.LogError($"Queue '{TargetQueue}' is not available");"

This reverts commit 663f5c6.

Tohron self-assigned this Sep 25, 2019

Tohron force-pushed the handle_disabled_queues branch from 8b38066 to d727a3e Compare September 30, 2019 23:39

MattGal reviewed Oct 1, 2019

View reviewed changes

MattGal requested changes Oct 1, 2019

View reviewed changes

alexperovich reviewed Oct 1, 2019

View reviewed changes

MattGal reviewed Oct 1, 2019

View reviewed changes

Tohron force-pushed the handle_disabled_queues branch from 59d49aa to 5139188 Compare October 1, 2019 21:03

Tohron commented Oct 1, 2019

View reviewed changes

Tohron force-pushed the handle_disabled_queues branch 2 times, most recently from 337d1e9 to ecbd195 Compare October 1, 2019 22:39

MattGal reviewed Oct 1, 2019

View reviewed changes

Aaron Kunkle added 3 commits October 3, 2019 11:01

More formatting

bc5fa28

Improved readability

6f2ddb8

Renamed boolean

8a0d34d

Tohron force-pushed the handle_disabled_queues branch from e7ec214 to 8a0d34d Compare October 3, 2019 18:01

MattGal approved these changes Oct 3, 2019

View reviewed changes

Tohron merged commit 663f5c6 into master Oct 3, 2019

ChadNedzlek added a commit to ChadNedzlek/arcade that referenced this pull request Oct 4, 2019

Revert "Helix SDK Avoids Upload for Disabled Queues (dotnet#4011)"

fd9e31f

This reverts commit 663f5c6.

ChadNedzlek added a commit that referenced this pull request Oct 4, 2019

Revert "Helix SDK Avoids Upload for Disabled Queues (#4011)" (#4060)

ca919f2

This reverts commit 663f5c6.

riarenas deleted the handle_disabled_queues branch January 28, 2020 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helix SDK Avoids Upload for Disabled Queues #4011

Helix SDK Avoids Upload for Disabled Queues #4011

Tohron commented Sep 25, 2019 •

edited

Loading

dnfclas commented Sep 25, 2019 •

edited

Loading

Tohron commented Sep 25, 2019

Tohron commented Sep 26, 2019

MattGal Oct 1, 2019

MattGal Oct 1, 2019

MattGal Oct 1, 2019

Tohron Oct 1, 2019

MattGal Oct 1, 2019

MattGal left a comment

alexperovich Oct 1, 2019

alexperovich Oct 1, 2019

MattGal Oct 1, 2019

Tohron Oct 1, 2019

MattGal Oct 1, 2019

Tohron Oct 1, 2019

Tohron Oct 1, 2019

MattGal Oct 1, 2019

MattGal left a comment

MattGal Oct 3, 2019 •

edited

Loading

		@@ -23,7 +23,7 @@ namespace Microsoft.Cci.Extensions.CSharp
		public static class CSharpCciExtensions

Helix SDK Avoids Upload for Disabled Queues #4011

Helix SDK Avoids Upload for Disabled Queues #4011

Conversation

Tohron commented Sep 25, 2019 • edited Loading

dnfclas commented Sep 25, 2019 • edited Loading

Tohron commented Sep 25, 2019

Tohron commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattGal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattGal left a comment

Choose a reason for hiding this comment

MattGal Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Tohron commented Sep 25, 2019 •

edited

Loading

dnfclas commented Sep 25, 2019 •

edited

Loading

MattGal Oct 3, 2019 •

edited

Loading