-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Publish SDK installer artifacts in CI pipeline #52233
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
Open
Copilot
wants to merge
4
commits into
main
Choose a base branch
from
copilot/publish-sdk-artifact-ci-pipeline
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+11
−0
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
So, this will double publish. Meaning, adding this here will cause all builds to publish these assets. Internal builds already publish these assets via the
enablePublishBuildAssetsand the publish-build-assets.yml template:sdk/.vsts-ci.yml
Lines 291 to 306 in 2b7468d
Wait, reading this, we moved to
isAssetlessBuild. @ViktorHofer Does the internal build publish assets? Because adding this will now publish assets and I believed our goal was to no longer publish assets.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.
Assetless-build PR for reference: #49113
Uh oh!
There was an error while loading. Please reload this page.
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.
Also wondering if we can just use the Arcade mechanism to publish assets instead of doing it ourselves. Meaning, use the publish-build-assets.yml template instead in the
.vsts-pr.ymlpipeline directly. Then, it would only happen in external PR builds.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.
Yes, sdk shouldn't publish anything in the official assetless build.
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.
Btw, I see that sdk still builds in the official build. I think that was necessary back a few months ago when we didn't preserve intermediates for compliance scanning tools but that got solved meanwhile. Most/all? other repos that enabled assetless builds don't build anymore.
https://dev.azure.com/dnceng/internal/_build/results?buildId=2863248&view=results
@mmitche do you remember if that was indeed the reason when you made the switch?
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.
Yes, OneLocBuild should still run in individual repo's builds. Compliance/SDL now runs centrally in the VMR and there's an owner to transfers items to the individual teams. Regarding shipping, no - sdk shouldn't ship any artifacts. I'm pretty sure it doesn't ship anything today as isAssetlessBuild is set to true which skips all that.
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.
Unless the VMR copies a repo's pipeline configuration for SDL/Compliance, I would not trust SDL tool runs from the VMR as they're likely misconfigured. For example, BinSkim requires these glob filters to run properly on this repo:
sdk/eng/pipelines/templates/jobs/sdk-build.yml
Lines 60 to 63 in 6085c30
Other repos would have specific files for filters or other logic in specific files within the repo that would be declared in the
sdlsection of the 1ES template. This repo doesn't have anything like that but it would exist here:sdk/.vsts-ci.yml
Lines 73 to 87 in 6085c30
@RikkiGibson You're still shipping the source package you added as part of our official build pipeline, right?
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 source package should also be produced from the VMR and should be shipped from there. Likely there's no material difference, but the VMR is the source of truth
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 VMR is correctly configured. We have an active stream of binskim / CG issues. We are in close contact with the compliance folks in Prague an Redmond and are using a preview feature that does operate on nuget packages and archives instead which makes the above glob unnecessary. @marcpopMSFT knows about all this.
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.
@MiYanni Binkskim in the VMR scans the output artifacts that are uploaded, rather than the. It utilizes binskim's newer functionality to unpack archives. I don't think it needs to be run in the SDK build any longer.