Skip to content
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

Adding Installation Check in Pre-Release Stage #23575

Merged
merged 3 commits into from
Aug 30, 2021
Merged

Conversation

ckairen
Copy link
Member

@ckairen ckairen commented Aug 26, 2021

Doing a blank slate installation pre-release to make sure the dependencies are correct, and the package that's about to release is able to restore.

Pipeline Ref:
lines 961~979
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1067279&view=logs&j=ba5a289f-2f09-530e-afa5-59b09a10fdf0&t=50abcb8c-011d-55a9-3c4a-d42bfcc3eecb&l=961

https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1067279&view=logs&j=8a59aa26-f1cb-5dea-999b-76ab4b63e138&t=009a8b76-fd91-58d4-0e63-224c4153136f&l=515

Issue described here Azure/azure-sdk-tools#1841

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

  • Please open PR in Draft mode if it is:
    • Work in progress or not intended to be merged.
    • Encountering multiple pipeline failures and working on fixes.
  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • I have read the contribution guidelines.
  • The pull request does not introduce breaking changes.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

  • Include updated management metadata.
  • Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

  • If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.

  • If the check fails at the Verify Code Generation step, please ensure:

    • Do not modify any code in generated folders.
    • Do not selectively include/remove generated files in the PR.
    • Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
      Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.

    Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:

      `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`
    

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

Comment on lines 52 to 53
- pwsh:
mkdir InstallationCheck
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- pwsh:
mkdir InstallationCheck
- pwsh: mkdir InstallationCheck

Copy link
Member

@benbp benbp left a comment

Choose a reason for hiding this comment

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

Some nits but LGTM.

@ckairen ckairen force-pushed the albertcheng/dep-check branch from b7dda0e to 4fbb886 Compare August 30, 2021 19:54
@ckairen ckairen force-pushed the albertcheng/dep-check branch from 4fbb886 to 8d660b4 Compare August 30, 2021 19:57
@ckairen ckairen merged commit fe1a256 into main Aug 30, 2021
@ckairen ckairen deleted the albertcheng/dep-check branch August 30, 2021 21:07
@@ -43,6 +43,18 @@ stages:
dependsOn: Signing
condition: and(succeeded(), ne(variables['SetDevVersion'], 'true'), ne(variables['Skip.Release'], 'true'), ne(variables['Build.Repository.Name'], 'Azure/azure-sdk-for-net-pr'))
jobs:
- job: InstallationCheck
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this is a separate job?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the installation check should be done before the other jobs and this just doesn't quite fit into the definition of "CreateReleaseTag." I thought about putting this as steps within PublishPackage. Do you think this approach would be better?

Copy link
Member

Choose a reason for hiding this comment

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

I would like it to fail as early in the process as possible. I would probably put it with the tagging job as we do some other checks there as well. Doing just this in a separate job causes us to use another agent and also causes more overhead that probably isn't necessary just for the one check.

Write-Host "dotnet nuget add source $localFeed"
dotnet nuget add source $localFeed

$version = (Get-ChildItem "$localFeed/*.nupkg" -Exclude "*.symbols.nupkg" -Name).replace(".nupkg","").replace("$Artifact.","")
Copy link
Member

Choose a reason for hiding this comment

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

While this might work in some cases it isn't the easiest or best way to get the version number. We always publish a packageinfo.json file in the artifacts as well that we generally use to read the metadata and version of the artifacts.

dotnet new console
$localFeed = "$PipelineWorkspace/$ArtifactsDirectory-signed/$Artifact"
Write-Host "dotnet nuget add source $localFeed"
dotnet nuget add source $localFeed
Copy link
Member

Choose a reason for hiding this comment

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

Does the source need to e added given that you are explicitly pass it to the restore command?

}

Write-Host "dotnet nuget locals all --clear"
dotnet nuget locals all --clear
Copy link
Member

Choose a reason for hiding this comment

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

If you use --packages to isolate the packages you shouldn't need to call this clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't we still need to clear the cache? (especially when doing the retries here #23739

Copy link
Member

Choose a reason for hiding this comment

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

When you use an independent packages folder it should only uses packages from that path. For retries you can just delete that directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw ill just push those changes along with PR for this #23739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packages should only be released if they will install successfully
4 participants