Give a warning before trying to publish symbols-only packages #1795
Conversation
Co-authored-by: Maria Zhelezova <43066499+mazhelez@users.noreply.github.com>
…rup/symbolspackage
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…rup/symbolspackage
…up1/al-go into aholstrup/symbolspackage
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Actions/RunPipeline/RunPipeline.psm1
Outdated
|
|
||
| try { | ||
| # Create folder in temp directory with a unique name | ||
| $tempFolder = Join-Path $RunnerTempFolder "DevelopmentTools-$(Get-Random)" |
There was a problem hiding this comment.
Why not installing the tool locally?
For better experience in the self-hosted runner case, consider reusing the folder and checking if the tool is already installed.
There was a problem hiding this comment.
My concern with doing that was mainly around what will happen when we update the package version in AL-Go. We'd need some logic that can update the package version on-the-fly. Seemed easier and less risky just to install it in a temp folder that is only used for that job.
There was a problem hiding this comment.
dotnet tool update?
You can also still use --tool-path and install the higher version when needed (a folder per version?).
There was a problem hiding this comment.
Also, I recommend separating the installation of Microsoft.Dynamics.BusinessCentral.Development.Tools in another function.
There was a problem hiding this comment.
dotnet tool update?
You can also still use --tool-path and install the higher version when needed (a folder per version?).
I tried to avoid installing the tool globally (i.e. dotnet tool install --global) as I'm not totally sure how it would work on self-hosted runners and whether there could be some caveats.
Using a specific tool-path on the runner that we keep around from build-to-build could be an option but again could have some caveats:
- What happens if/when the file is corrupted? We've seen that happen with BCArtifacts for example.
- Could we run into file locks if multiple runners are requesting it at the same time?
- Once we update to a new version, should AL-Go go and clean up the old folder?
Installing it in a temp folder unique to each runner seemed like the less risky approach to me.
Also, I recommend separating the installation of Microsoft.Dynamics.BusinessCentral.Development.Tools in another function.
Will do. I think we have similar logic for installing the sign tool actually so I can refactor these to use the same function.
| Should -Invoke -CommandName 'OutputWarning' -Times 1 -ModuleName RunPipeline -ParameterFilter { $Message -like "*App EssentialBusinessHeadlinesSymbols.app is a symbols package and should not be published. The workflow may fail if you try to publish it." } | ||
|
|
||
| # Assert that Trace-Warning was not called | ||
| Assert-MockCalled Trace-Warning -Exactly 0 -ModuleName RunPipeline |
There was a problem hiding this comment.
Why not trace the warning actually?
There was a problem hiding this comment.
I only trace a warning to telemetry if there is some internal error in the check. This asserts that it didn't fail during the analysis.
There was a problem hiding this comment.
That's what I mean: why not also trace a warning also when symbols are about to be published?
| if ($finalUrl -like 'http*://*') { | ||
| # Check validity of URL | ||
| try { | ||
| Invoke-WebRequest -Method Head -UseBasicParsing -Uri $finalUrl | Out-Null |
There was a problem hiding this comment.
This check was added to fail early, if case the URL isn't accessible.
Now that apps are downloaded a couple of lines below, this check isn't necessary.
| Should -Invoke -CommandName 'OutputWarning' -Times 1 -ModuleName RunPipeline -ParameterFilter { $Message -like "*App EssentialBusinessHeadlinesSymbols.app is a symbols package and should not be published. The workflow may fail if you try to publish it." } | ||
|
|
||
| # Assert that Trace-Warning was not called | ||
| Assert-MockCalled Trace-Warning -Exactly 0 -ModuleName RunPipeline |
There was a problem hiding this comment.
That's what I mean: why not also trace a warning also when symbols are about to be published?
3 participants
❔What, Why & How
We've seen a couple of issues where builds are failing because one of the installApps is a symbols-only package. We could improve a bit here by displaying a warning when people try to do this. Eventually, this should probably be an error but showing a warning is a good first step IMO.
Related to issue: #1512
✅ Checklist