Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Join-Verticals task, use it at the final Join Point #19369
Add Join-Verticals task, use it at the final Join Point #19369
Changes from 3 commits
64b685f
3b0b615
c2ee890
76855fe
cc3ba36
c0ec4d4
44028e6
cb0c2f9
2d268a2
69e6f1f
2810fb2
645af11
f5b4bcc
39bad4e
9de8ec1
5d87fcd
3d35720
0531668
6bceba3
0782cda
7dfafd6
749b4bf
4c3db41
150cd78
fb3b996
f9936a1
586e019
f703e79
ebfb759
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 spec uses
primaryDependentJob
as the YML variable name: https://github.com/dotnet/arcade/blob/main/Documentation/UnifiedBuild/Unified-Build-Join-Point.mdThere 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.
I renamed the yml parameter, I didn't change msbuild property name tho
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.
We can also update the spec if you think
mainVertical
makes more sense.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.
I don't mind keeping this as is, I'd just prefer to keep
MainVertical
in the task, since job is an AzDo thing, while Vertical is a VMR conceptThere 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.
Same question as in vmr-build-pr.yml. Why don't we define this in the stages/vmr-build.yml?
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.
I did some refactoring, renamed the stage to VMR Post Build for better clarity. I think with that name it becomes clear that it should be a separate yml. Let me know if you think differently please
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.
I would call this project "publish-final.proj" and the target "PublishFinal":
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.
I don't think we should call it
publish-final
, as this will also be used to download artifacts for join point builds like we discussed. I named itjoin-verticals
since it can join any number of verticals, including all of themThere 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.
I was under the impression that we can't directly use this target for the join but if we can, great.
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.
We should be able to, there's a property called
VerticalSubSet
that has a list of verticals we want to join, so it doesn't have to be the full set. On second thought, this isn't a very good name, perhapsVerticalsToJoin
is better.There's also a
FlatCopy
property, that tells the task how we want it to copy artifacts, if it's set to true, all packages will be in a flat layout in the packages folder, and if it's not, we'll keep the same layout as it is in the artifacts, so for the non final join points we should keep it false. Same goes for assetsThere 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.
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 functionality probably needs a try/catch w/retry on certain responses.
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.
A common pattern is
EnsureSuccessStatusCode()
here with the corresponding try/catch and retryThere 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.
In addition a number of these objects (e.g. HttpResponseMessage) are IDisposable and should have
using
blocks.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 need/purpose of this method isn't super obvious. Can you add a comment as to why this is written this way?