-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Courtesy Push Pipeline Improvement - Part#1 | Eliminate Job-4 build_all_tasks_for_deployments #21417
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?
Courtesy Push Pipeline Improvement - Part#1 | Eliminate Job-4 build_all_tasks_for_deployments #21417
Conversation
| - script: | | ||
| rmdir /s /q _build | ||
| rmdir /s /q _package\nested-zips-layout | ||
| displayName: Remove _build folder to reduce space |
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.
Should we add this as well to the build-all-steps.yml to reduce space after build is complete??
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 clean step wasn’t part of the first job even before my changes. I believe it was introduced due to the double-build scenario. Historically, we haven’t seen any space-related issues in the first job, so I don’t intend to add it there.
|
|
||
| const hotfix = process.argv[2].toLowerCase() == 'true'; | ||
| const hotfix = process.argv[2] ? process.argv[2].toLowerCase() == 'true' : false; | ||
| const individually = process.argv[3] == 'individually'; |
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.
should we also add a similar null/undefined check for 'individually' as well to maintain consistency??
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.
This null check is required because of the toLowerCase() call—if the value is null, it will fail. For the other variable, we aren’t performing any operations, so the existing code can handle null cases for variable individually.
| $downloadUrl = "$($env:SYSTEM_TEAMFOUNDATIONCOLLECTIONURI)$env:SYSTEM_TEAMPROJECTID/_apis/build/builds/$env:BUILD_BUILDID/artifacts?artifactName=package&%24format=zip&api-version=3" | ||
| $tasksBuildArtifact = $env:TASKS_BUILD_ARTIFACT | ||
| $downloadUrl = "$($env:SYSTEM_TEAMFOUNDATIONCOLLECTIONURI)$env:SYSTEM_TEAMPROJECTID/_apis/build/builds/$env:BUILD_BUILDID/artifacts?artifactName=$tasksBuildArtifact&%24format=zip&api-version=3" | ||
| $downloadTarget = "$env:SYSTEM_ARTIFACTSDIRECTORY\package.zip" |
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.
Hi @dassayantan24 , now we are removing package.zip as shown in below image. So please check whether we need to update this to $downloadTarget = "$env:SYSTEM_ARTIFACTSDIRECTORY\$tasksBuildArtifact.zip"
And, please add work item in the descripton.
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.
This step is a follow-up where we download the artifact published from the previous job. In the first job, we publish a package as an artifact named allTasks. Now, in this step, we are downloading that package, and inside the context, it is referred to as package.zip.
The first two lines of code specify the download location, which needs to be changed. However, the third line defines the name under which we want to download the package. This name can be anything, so it does not need to be changed.
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.
Work Item Added
|
|
||
| // Parse command line arguments (positional) | ||
| // Usage: node stage-milestone.js <tasksBuildArtifact> | ||
| var tasksBuildArtifact = process.argv[2] || 'package'; |
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.
tasksBuildArtifact default value is set to alltasks, shouldn't here as well default value should be package?
Context
The idea is to reduce the execution time of the Courtesy Push pipeline by eliminating the Job - build_all_tasks_for_deployments where we were rebuilding the same set of tasks.
Work-Item - https://dev.azure.com/mseng/AzureDevOps/_workitems/edit/2329703
Design Doc - Courtesy Push Pipeline Improvement - latest.docx
Task Name
Description
Pipeline Architecture Update:
Simplification Changes:
Risk Assessment (Low / Medium / High)
Assess the level of risk and provide reasoning (e.g., scope, impact, backward compatibility).
High , This PR includes changes that can directly impact artifacts used in the task release.
Change Behind Feature Flag (Yes / No)
Can this change be behine feature flag, if not why?
No
Tech Design / Approach
Documentation Changes Required (Yes/No)
Indicate whether related documentation needs to be updated. No
Unit Tests Added or Updated (Yes / No)
Indicate whether unit tests were added or modified to reflect these changes.
No
Additional Testing Performed
List all other tests performed (manual or automated, including integration, regression, scenario tests, etc.).
Manually Tested , https://dev.azure.com/mseng/AzureDevOps/_build/results?buildId=30694336&view=results
Logging Added/Updated (Yes/No)
Telemetry Added/Updated (Yes/No)
Rollback Scenario and Process (Yes/No)
Dependency Impact Assessed and Regression Tested (Yes/No)
Checklist