-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Canary task rollouts #21113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Canary task rollouts #21113
Conversation
This reverts commit 6a41025.
\azp run |
/azp run |
\azp run |
/azp run |
\azp run |
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for semver-based task versions and canary (A/B) rollouts by introducing a useSemverBuildConfig
flag throughout the build scripts, CI definitions, and the BuildConfigGen tool.
- Extend
make.js
/make-util.js
to accept and propagate a new--use-semver-build-config
option. - Update Azure Pipelines YAML (
ci/*
andazure-pipelines.yml
) to branch on the new flag and optionally generate release notes. - Enhance BuildConfigGen (
VersionParser
,TaskVersion
,Program
, and launch configs) to parse, format, and emit a “build” suffix for semver rollouts.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
make.js | Pass new useSemverBuildConfig CLI flag. |
make-util.js | Add useSemverBuildConfig parameter and logic. |
ci/build-all-tasks.yml | Condition and script steps for semver flag. |
ci/build-all-steps.yml | Add useSemverBuildConfig /generateReleaseNotes parameters and conditions. |
azure-pipelines.yml | Expose new parameters to downstream templates. |
Tasks/PowerShellV2/task.loc.json & task.json | Bump patch to 1 for new release. |
BuildConfigGen/VersionParser.cs | Introduce TryParseVersionComponents for semver. |
BuildConfigGen/TaskVersion.cs | Support “build” suffix, update ToString logic. |
BuildConfigGen/Program.cs | Propagate semver flag into MainInner and tasks. |
BuildConfigGen/Properties/launchSettings.json | Add semver launch profile. |
BuildConfigGen/Debugging/VsCodeLaunchConfigGenerator.cs | Include Build segment when generating debug configs. |
Comments suppressed due to low confidence (1)
ci/build-all-tasks.yml:43
- [nitpick] The trailing semicolon in the script command is unnecessary and inconsistent with other steps. Consider removing it for consistency.
- script: node make.js serverBuild --task "$(task_pattern)" $(includeLocalPackagesBuildConfigParameter) --useSemverBuildConfig true;
} | ||
|
||
return String.Format(CultureInfo.InvariantCulture, "{0}.{1}.{2}{3}", Major, Minor, Patch, suffix); | ||
return String.Format(CultureInfo.InvariantCulture, "{1}.{2}{3}", Major, Minor, Patch, suffix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format string omits the Major component. Change placeholders to "{0}.{1}.{2}{3}" so Major is included in the output.
return String.Format(CultureInfo.InvariantCulture, "{1}.{2}{3}", Major, Minor, Patch, suffix); | |
return String.Format(CultureInfo.InvariantCulture, "{0}.{1}.{2}{3}", Major, Minor, Patch, suffix); |
Copilot uses AI. Check for mistakes.
{ | ||
String suffix = String.Empty; | ||
if (IsTest) | ||
{ | ||
suffix = "-test"; | ||
throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing NotImplementedException when IsTest
is true will crash at runtime. Either implement the test-suffix behavior or remove this exception to avoid unexpected failures.
throw new NotImplementedException(); | |
suffix = "-test"; |
Copilot uses AI. Check for mistakes.
@@ -46,7 +46,7 @@ internal static class Config | |||
{ | |||
public static readonly string[] ExtensionsToPreprocess = new[] { ".ts", ".json" }; | |||
|
|||
public record ConfigRecord(string name, string constMappingKey, bool isDefault, bool isNode, string nodePackageVersion, bool isWif, string nodeHandler, string preprocessorVariableName, bool enableBuildConfigOverrides, bool deprecated, bool shouldUpdateTypescript, bool writeNpmrc, string? overriddenDirectoryName = null, bool shouldUpdateLocalPkgs = false, bool useGlobalVersion = false, bool useAltGeneratedPath = false, bool mergeToBase = false); | |||
public record ConfigRecord(string name, string constMappingKey, bool isDefault, bool isNode, string nodePackageVersion, bool isWif, string nodeHandler, string preprocessorVariableName, bool enableBuildConfigOverrides, bool deprecated, bool shouldUpdateTypescript, bool writeNpmrc, string? overriddenDirectoryName = null, bool shouldUpdateLocalPkgs = false, bool useGlobalVersion = false, bool useAltGeneratedPath = false, bool mergeToBase = false, bool abTaskReleases = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is default value true here ? and where is it being used for now. I dont see any reference to abTaskReleases later anywhere ? should we set this to default false.
Context
This shows the changes that we must make on the pipeline tasks side to allow canary rollouts
Task Name
NA
Description
This shows the changes that we must make on the pipeline tasks side to allow canary rollouts. Adds a new field release in the task to associate task with sprint versions
Risk Assessment (Low / Medium / High)
Low (Flag based check, for older task no changes are required)
Unit Tests Added or Updated (Yes / No)
No ( changes are in task build process)
Additional Testing Performed
Manual testing to generate tasks in new format
Documentation Changes Required (Yes / No)
Yes
Checklist