Skip to content

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

sanjuyadav24
Copy link
Contributor

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

  • Related issue linked (if applicable)
  • Task version was bumped — see versioning guide
  • Verified the task behaves as expected

@sanjuyadav24 sanjuyadav24 requested review from tarunramsinghani and a team as code owners June 26, 2025 08:21
@sanjuyadav24
Copy link
Contributor Author

\azp run

@sanjuyadav24
Copy link
Contributor Author

/azp run

@sanjuyadav24
Copy link
Contributor Author

\azp run

@sanjuyadav24
Copy link
Contributor Author

/azp run

@sanjuyadav24
Copy link
Contributor Author

\azp run

@sanjuyadav24
Copy link
Contributor Author

/azp run

@raujaiswal raujaiswal requested a review from Copilot June 30, 2025 07:04
Copy link

@Copilot Copilot AI left a 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/* and azure-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);
Copy link
Preview

Copilot AI Jun 30, 2025

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.

Suggested change
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();
Copy link
Preview

Copilot AI Jun 30, 2025

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.

Suggested change
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);
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants