Skip to content

Conversation

@lbussell
Copy link
Member

@lbussell lbussell commented Nov 11, 2022

@lbussell lbussell requested a review from a team as a code owner November 11, 2022 17:41
@lbussell lbussell force-pushed the security-partners-ci branch from 41c02f4 to 77a500f Compare November 11, 2022 18:20
@lbussell
Copy link
Member Author

@MichaelSimons trying a new approach: remove all nuget configs from all submodules. I don't think it will cause any major build issues. Let me know what you think. (internal) CI run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2043133&view=results

@@ -0,0 +1,19 @@
variables:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid putting content at the root of the tarball. This should go into the eng\pipelines folder IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no existing eng/pipelines folder that I see. Do you mean to put it in <installer src>/eng/... or <tarball content>/eng/...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended to create the folder in <tarball content>/eng/. I would be fine with just putting it in <tarball content>/

@@ -0,0 +1,19 @@
variables:
NugetSecurityAnalysisWarningLevel: none
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this variable used? From your feedback, it doesn't seem to. I think it is worth reaching out to [email protected] on why this doesn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the problem is not multiple feeds rather there is no clear first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this variable does nothing and might have been a placeholder while teams fix their NuGet configs. I have sent an email to that alias and CC'ed you, and I will remove the variable from the pipeline as well.

@@ -0,0 +1,19 @@
variables:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pipeline need a PR trigger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PR validation on Azure Repos Git, it's configured via the UI and not yaml: https://learn.microsoft.com/en-us/azure/devops/pipelines/repos/azure-repos-git?view=azure-devops&tabs=yaml#pr-triggers

I'll put a trigger: none here for now, but do you think we should have it trigger on commits to release/* branches as well as for PR validation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't realize that.

@lbussell
Copy link
Member Author

lbussell commented Nov 15, 2022

For some reason the online smoke tests have failed, but only after I fixed the installer nuget config path. @MichaelSimons could you take a look, given you're more familiar with the smoke test nuget config setup? https://dev.azure.com/dnceng/internal/_build/results?buildId=2043133&view=results (internal link)

inputs:
filePath: $(Build.SourcesDirectory)/eng/common/SetupNugetSources.sh
arguments: $(Build.SourcesDirectory)/NuGet.config $Token
filePath: $(installerSrcDir)/eng/common/SetupNugetSources.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem with the failing tests is that in an offline build, we strip the online feeds from the repo's nuget configs - https://github.com/dotnet/installer/blob/main/src/SourceBuild/tarball/content/repos/Directory.Build.targets#L184. Additionally, we don't want to be adding these sources for the installer's nuget.config we build with. We really only want to manipulate the nuget.config used for testing. I'll let you propose how to accomplish that.

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly reminder to proactively ping the thread on the nuget security scanning issue.

@lbussell lbussell changed the title [release/6.0.1xx] Add CI for dotnet-security-partners [release/6.0.1xx] Add CI for security-partners-dotnet Nov 16, 2022
@lbussell
Copy link
Member Author

I submitted dotnet/source-build#3125 as a draft in case we don't get an exception to the NuGet Security Analysis check.

@lbussell lbussell merged commit 9299e47 into dotnet:release/6.0.1xx Nov 17, 2022
MichaelSimons added a commit that referenced this pull request Nov 30, 2022
* [release/6.0.1xx] Add CI for security-partners-dotnet (#14963)

* Add CI for dotnet-security-partners

* Remove nuget configs from all repo submodules

* Address code review

* Move security-partners ci to tarball/eng dir

* Copy out installer NuGet config before build

* Fix relative path for installer template

* Update src/SourceBuild/Arcade/eng/common/templates/job/source-build-build-tarball.yml

Co-authored-by: Michael Simons <[email protected]>

* Revert "Remove nuget configs from all repo submodules"

This reverts commit 87b8a2c.

Co-authored-by: Michael Simons <[email protected]>

* Update branding to 6.0.113

Co-authored-by: Logan Bussell <[email protected]>
Co-authored-by: Michael Simons <[email protected]>
Co-authored-by: Sean Reeser <[email protected]>
Co-authored-by: Marc Paine <[email protected]>
Co-authored-by: Matt Mitchell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants