Simplify pipelines#27505
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (243 lines, 9 files), I've queued these reviewers:
How this works
|
| requireNotice: ${{ parameters.shouldReleaseDockerImage }} | ||
|
|
||
| - ${{ if eq(parameters.packageManager, 'pnpm') }}: | ||
| - ${{ if ne(parameters.skipPackageInstall, true) }}: |
There was a problem hiding this comment.
Does this work?
| - ${{ if ne(parameters.skipPackageInstall, true) }}: | |
| - ${{ if not(parameters.skipPackageInstall) }}: |
There was a problem hiding this comment.
Same for other eq and ne locations below
There was a problem hiding this comment.
Correct. Fixed them all.
|
|
||
| if [[ "${{ parameters.packageManager }}" == "none" ]] && [[ "${{ parameters.setVersion }}" == "True" ]]; then | ||
| echo "##vso[task.logissue type=error]packageManager: 'none' is incompatible with setVersion: true (version-setting transitively requires a package manager)." | ||
| if [[ "${{ parameters.skipPackageInstall }}" == "true" ]] && [[ "${{ parameters.setVersion }}" == "True" ]]; then |
There was a problem hiding this comment.
Should this be True rather than true?
There was a problem hiding this comment.
The code above uses both, which I suppose is likely a bug. I'll look into this more.
There was a problem hiding this comment.
Looks like the code using lowercase was bugged, and that validation could never trigger. Docs for this are in https://learn.microsoft.com/en-us/azure/devops/pipelines/process/expressions?view=azure-devops#boolean-1 . Fixed here and there.
| type: boolean | ||
| default: false | ||
|
|
||
| - name: isBundleSizeArtifactsPipeline |
There was a problem hiding this comment.
Might be preferable to leave the bundle size stuff for now, Tommy and I are making changes in this area and some of this is probably going to change anyway. Since the bundle size artifacts live in the client-side of the world it may make more sense for them to be produced over here...?
There was a problem hiding this comment.
Recently you updated the bundle size stuff in the other pipeline, the one we actually use for it, and left this one out of date. I find having the code duplicated here where it is dead code that's not fully up to date confusing. I do agree that it is probably better to not touch that in this PR though, as that's a whole different mess that the rest of these changes.
There was a problem hiding this comment.
I have reverted my changes in this area
| parameters: | ||
| buildDirectory: ${{ variables.FluidFrameworkDirectory }}/docs | ||
|
|
||
| - task: Bash@3 |
There was a problem hiding this comment.
Was this just duplicate?
There was a problem hiding this comment.
This pretty much just an inline copy of include-install.yml (except for that pwd statement I added at some point) which it now just uses.
Description
Simplify pipelines a bit.
Reviewer Guidance
The review process is outlined on this wiki page.