-
Notifications
You must be signed in to change notification settings - Fork 180
Bug fixes for running a large/complicated solution on AL-Go #2075
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: main
Are you sure you want to change the base?
Conversation
|
@microsoft-github-policy-service agree |
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
This PR addresses several issues to enable moving Bunker Holding Group to AL-Go for GitHub, including bug fixes for dependency resolution, artifact handling for workflow reruns, build order optimization to reduce pipeline execution time, error handling improvements, and spelling corrections.
Changes:
- Fixed spelling mistakes in variable names (dependecies → dependencies)
- Added error handling to PipelineCleanup.ps1 to prevent workflow failures when containers aren't used
- Improved dependency resolution to correctly handle projects with only Apps or only TestApps
- Fixed artifact retrieval to handle duplicate artifacts from workflow reruns by selecting the latest version
- Optimized build order to defer projects without dependents to later stages, reducing overall pipeline time
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Actions/PipelineCleanup/PipelineCleanup.ps1 | Added try-catch error handling to prevent failures when containers aren't present |
| Actions/DownloadProjectDependencies/DownloadProjectDependencies.Action.ps1 | Fixed spelling: dependecies → dependencies |
| Actions/Github-Helper.psm1 | Fixed dependency resolution logic and artifact handling for workflow reruns |
| Actions/AL-Go-Helper.ps1 | Optimized build order and changed defaultBcContainerHelperVersion to a specific fork |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@microsoft-github-policy-service agree |
|
Interesting idea on postponing jobs to actually save time overall. E.g., the following build order flowchart TD
A[Build apps, Project A] --> B[Build apps, Project B]
A[Build apps, Project A] --> C[Build apps, Project C]
B --> D[Test apps, Project A]
B --> E[Test apps, Project B]
will become flowchart TD
A[Build apps, Project A] --> B[Build apps, Project B]
B --> D[Test apps, Project A]
B --> E[Test apps, Project B]
B --> C[Build apps, Project C]
That way compilation errors might have been earlier, saving time and compute. My suggestion is to try out adding project dependencies explicitly. Let me know what you think about this idea. |
Your example above is wrong, if you had that order of dependencies, the matrix would look like this in the current version of AL-Go: flowchart TD
A[Build apps, Project A] --> B[Build apps, Project B]
A[Build apps, Project A] --> C[Build apps, Project C]
A[Build apps, Project A] --> D[Test apps, Project A]
B --> E[Test apps, Project B]
Causing the test of B to be delayed until the test of A is done, that is exactly what I want to avoid. I would very much warn against explicit project dependency definitions in AL-Go, it complicates things a lot - I think build order needs to be calculated based on dependencies - else adding a dependency to an app.json or a new app will cause you to need to manually re-think build order in some structure. In Bunker we have many projects and many apps - this could become a nightmare. What I could add to this PR instead would be to add a project setting called: PostponeProjectInBuildOrder, which you could set to true on projects, which would be allowed to be scheduled later (if possible, i.e. if nobody depends on it). This way, I would set that on all test projects and the build projects would not be deferred, but built at the earliest time, essentially causing the result to be: flowchart TD
A[Build apps, Project A] --> B[Build apps, Project B]
A[Build apps, Project A] --> C[Build apps, Project C]
B --> D[Test apps, Project A]
B --> E[Test apps, Project B]
Which is exactly what we want (and better than my current suggestion) I have added this property to the PR - can remove again if needed. @mazhelez - what do you think about that property? (default is false causing the build order to be like before) |
|
@freddydk please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|

❔What, Why & How
This PR fixes a few minor bugs and 3 major, all changes required for moving Bunker Holding Group to AL-Go for GitHub
[Bug] Fix a small spelling mistake in DownloadProjectDependencies.Action.ps1
[Bug] Avoid failing entire workflows due to cleanup issues (f.ex. if no Container is used) in PipelineCleanup.ps1
Fixes #2086 - In AL-Go-Helper.ps1
The sort order is changed from this:

to this:

Which completes in 2:01, saving 45 minutes from the first run, just by re-ordering and allowing people to see compile errors up-front - fail early!
The key difference here is just the order in which projects are built/tested. In the first run, build3 had to wait 30 minutes until the test run was done before building the next jobs, and 1 hour and 44 until the last build jobs would run.
In the second run - all test jobs are running in parallel at the very end, getting compile errors quicly and postponing test runs until the very last.
Subsequently, when using GitHub hosted Custom runners, we can achieve this:

Note that the custom runners are running tests in 40 minutes, where the GitHub hosted runners takes 104 minutes. This is because the GitHub hosted custom runners are using SQL Server Developer edition and no docker.
Saving a full 2 hours, bringing a complete build from 2 hours 46 minutes down to 46 minutes.
Fixes #2085 - In GetDependencies in GitHub-Helper.ps1
When downloading artifacts from projects build earlier in the same workflow, it flags projects as missing if either Apps or TestApps are missing. This means that projects, which only contains TestApps or Apps will be downloaded but also looked for in the last known good build later on. It doesn't fail, because it cannot find the artifacts in that built either.
See https://github.com/Freddy-DK/DependenciesProblems/actions/runs/21079606240/job/60630194330
Fixes #2084 in GetArtifactsFromWorkflowRun in GitHub-Helper.ps1
If a build is cancelled during tests or failing due to instability - and you subsequently re-run failed jobs (not all jobs) you will get multiple artifacts for succeeding projects and subsequent jobs cannot locate these artifacts (they get an array of artifacts). The "double" artifacts are not visible in the UI, but found when artifacts are queried. You could see this as a bug in GitHub, but we should fix this in AL-Go in a way that works, even if GitHub fixes their bug.
This build: https://github.com/Freddy-DK/DependenciesProblems/actions/runs/21090442931
Has double artifacts: https://api.github.com/repos/Freddy-DK/DependenciesProblems/actions/runs/21090442931/artifacts
When than downloading artifacts from prior builds for incremental builds, it fails with
See: https://github.com/Freddy-DK/DependenciesProblems/actions/runs/21090506962/job/60661024240
✅ Checklist