Skip to content

Publish EnsureAllPackagesExist.ps1 as artifacts#3926

Closed
heng-liu wants to merge 3 commits intodevfrom
dev-hengliu-CI-publishScript-EnsureNupkgsExist
Closed

Publish EnsureAllPackagesExist.ps1 as artifacts#3926
heng-liu wants to merge 3 commits intodevfrom
dev-hengliu-CI-publishScript-EnsureNupkgsExist

Conversation

@heng-liu
Copy link
Contributor

@heng-liu heng-liu commented Feb 26, 2021

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/667

Regression? Last working version:

Description

PR #3862 added a script to run in release pipeline. There is a list of nupkgs inside of this script, which is used to check if all nupkgs are created. The release pipeline gets the script from dev branch.
But the nupkgs list might change from time to time.
In order to make sure release pipeline gets the right list in the script, we publish the script EnsureAllPackagesExist.ps1 as artifacts.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@heng-liu heng-liu requested a review from a team as a code owner February 26, 2021 17:01
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

What do you think about changing the PR title to "Save scripts used in release scripts as build artifacts", and include TagNuGetClientGitRepostoryOnRelease.ps1. The comment above the task should also be changed if we do this.

Although there isn't a good technical reason why different branches would want different TagNuGetClientGitRepostoryOnRelease.ps1 files, In fact, if we do change how our builds are tagged, we would probably want old release branches to use the latest dev branch script, not the old copy in the release branch itself. But my gut reaction is that in general it's better for the release to use scripts from its own branch.

Alternatively, have you investigated finding the commit hash in the release pipeline? for build pipelines the predefined build variable Build.SourceVersion contains the git hash, so we could use https://raw.githubusercontent.com/nuget/nuget.client/$(Build.SourceVersion)/scripts/whatever. This way we wouldn't need to save any build artifacts, and it will work for old release branches that don't save this build artifact. I expect $(Build.SourceVersion) doesn't exist in a release pipeline, especially since a release pipeline can use multiple builds as input, so at the very least you have to specify which build you'd want the commit hash from.

Anyway, that's a different idea, although I think I like the build artifact more than generating a URL to github with the commit hash. But the two ideas are close to even for me.

PathtoPublish: "$(Build.Repository.LocalPath)\\scripts\\utils\\EnsureAllPackagesExist.ps1"
ArtifactName: "Scripts"
ArtifactType: "Container"
condition: "and(succeeded(),eq(variables['BuildRTM'], 'false'))"
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
condition: "and(succeeded(),eq(variables['BuildRTM'], 'false'))"
condition: "and(succeeded(), eq(variables['BuildRTM'], 'false'))"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing!
I investigated the other way of getting script from a specific commit hash and it should be workable.
We can either get commit hash from the variable you mentioned above, or from the buildinfo.json
Then according to getting-permanent-links-to-files, we can reference it like: https://github.com/NuGet/NuGet.Client/blob/18863da5be3dc8c7315f4416df1bc9ef96cb7446/scripts/utils/EnsureAllPackagesExist.ps1

Comparing the two ways, there is no difference for future builds.
For servicing old branches without the script EnsureAllPackagesExist.ps1, it looks like it's a difference of cherry-picking one commit or two commits. Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I lean towards using commit hash to read EnsureAllPackagesExist.ps1 file instead of publishing the script as a build artifact.

@nkolev92
Copy link
Member

This PR still relevant?

@heng-liu
Copy link
Contributor Author

heng-liu commented May 3, 2021

Yes, I'll try to address it this sprint.

@heng-liu heng-liu closed this Jun 7, 2021
@kartheekp-ms kartheekp-ms deleted the dev-hengliu-CI-publishScript-EnsureNupkgsExist branch June 8, 2021 01:00
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.

4 participants