Skip to content

Conversation

@tadelesh
Copy link
Member

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@tadelesh tadelesh requested a review from a team as a code owner August 20, 2021 12:38
@tadelesh
Copy link
Member Author

tested in personal repo
image
image

@tadelesh tadelesh requested a review from weshaggard August 23, 2021 05:30
$RELEASE_TITLE_REGEX = "(?<releaseNoteTitle>^\#+\s+(?<version>v$([AzureEngSemanticVersion]::SEMVER_REGEX))(\s+(?<releaseStatus>\(.+\))))"
$AUTOREST_VERSION_REGEX = "^module-version:\s+(?<version>$([AzureEngSemanticVersion]::SEMVER_REGEX))"

function Get-go-PackageInfoFromPackageFile ($pkg, $workingDirectory)
Copy link
Member

@chidozieononiwu chidozieononiwu Aug 23, 2021

Choose a reason for hiding this comment

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

Looks like this method can also serve as Get-go-PackageInfoFromRepo, since go only pulls from the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Go release process has no package process and all package info can get directly from repo. But in order to reuse 'create-tag-and-git-release.ps1' script in eng sys which use Get-go-PackageInfoFromPackageFile to get release info, I create this function by tradeoff.

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 definitely share and reuse this logic for both the packagefile and packageinfo.

@chidozieononiwu at some point I want to eliminate FromPackageFile function completely and either use PackageInfo or use the PackageInfo.json file we are generating as part of the build.

# get version from go config auterest.md
function Get-Version ($pkgPath)
{
$content = Get-Content(Join-Path $pkgPath "autorest.md")
Copy link
Member

@chidozieononiwu chidozieononiwu Aug 23, 2021

Choose a reason for hiding this comment

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

It looks like this only applies to a handfull of management packages. Should there also be logic for retrieving version from version.go files?

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 have changed the Get-Version method to support both data plane and mgmt plane. For mgmt plane of go track2, version can be obtained by autorest.md. For data plane, version can be obtained by version.go file. Maybe two version aquisition method can be unified if code gen add version.go to mgmt plane in the future. @jhendrixMSFT @ArcturusZhang

@check-enforcer
Copy link

This repository is protected by Check Enforcer. The check-enforcer check-run will not pass until there is at least one more check-run successfully passing. Check Enforcer supports the following comment commands:

  • /check-enforcer evaluate; tells Check Enforcer to evaluate this pull request.
  • /check-enforcer override; by-pass Check Enforcer (approvals still required).

@benbp
Copy link
Member

benbp commented Aug 25, 2021

A few powershell syntax nits (calling powershell functions via () is the equivalent to passing an array as a single argument and will break when args > 1). Otherwise looks good to me.

@tadelesh tadelesh force-pushed the tag_and_release_pipeline branch 2 times, most recently from aa2b872 to 2632eb8 Compare August 26, 2021 03:02
@tadelesh tadelesh requested a review from benbp August 26, 2021 03:03
Comment on lines 35 to 47
# get version from go config auterest.md
function Get-Version ($pkgPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

@chidozieononiwu How are we getting the version in go? I believe Joel and yourself settled on including it in a constants.go, this should be updated to include that.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. I am not sure the changes have been mad
e for all the client packages. However @tadelesh if its not too much trouble can you also support getting versions from constants.go file if version.go is absent.

Copy link
Member Author

@tadelesh tadelesh Aug 27, 2021

Choose a reason for hiding this comment

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

@seankane-msft @chidozieononiwu I've changed Get-Version method to the following logic to support several kinds of version set situation in go sdk:

  1. get all files with constants.go or version.go suffix under module folder
  2. find first string assignment statement in these files with semantic version sub-string by regex expression

@tadelesh tadelesh force-pushed the tag_and_release_pipeline branch 2 times, most recently from 9478617 to b94ca18 Compare August 27, 2021 03:41
@tadelesh tadelesh requested a review from seankane-msft August 27, 2021 03:42
@tadelesh tadelesh force-pushed the tag_and_release_pipeline branch from b94ca18 to b39491b Compare August 27, 2021 03:42
ServiceDirectory: ''

stages:
- ${{if and(eq(variables['Build.Reason'], 'Manual'), eq(variables['System.TeamProject'], 'internal'))}}:
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 this conditional is unnecessary since you're also including it in the template include for the archetype-sdk-client.yml file.

exit 1
}

# rewrite from artifact-metadata-parsing.ps1 as all 0.x.x version should be considered prerelease in go sdk
Copy link
Member

Choose a reason for hiding this comment

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

FYI I'm adding support for v0 versions in our core SemVer helper with Azure/azure-sdk-tools#1972. If that is included do we still need to duplicate this entire function? Hopefully not.

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 will be great that semver helper can handle this. I'll remove it after your pr merged and automatic sync done.

Copy link
Member

Choose a reason for hiding this comment

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

My SemVer change is in.

$Language = "go"
$packagePattern = "go.mod"
# rewrite from ChangeLog-Operations.ps1 used in Get-ChangeLogEntriesFromContent for go uses vx.x.x as version number
$RELEASE_TITLE_REGEX = "(?<releaseNoteTitle>^\#+\s+(?<version>v$([AzureEngSemanticVersion]::SEMVER_REGEX))(\s+(?<releaseStatus>\(.+\))))"
Copy link
Member

Choose a reason for hiding this comment

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

@chidozieononiwu we consider adding an option "v?" to our core regex instead of duplicating this. We do similar support for python in our Regex already which supports different prerelease labels.

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 wonder whether there's already a definitely plan for this version compatibility change. Could we use the rewrite workaround before the eventually solution cause there are lots of release work for mgmt plane sdk refreshment recently.

Copy link
Member

Choose a reason for hiding this comment

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

There isn't a plan for this currently and I think what you have is an acceptable workaround for now.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

If possible remove your CreateReleases function assuming it isn't needed after my SemVer change. Otherwise the changes look like a good starting point and we can tweak as we start to use it.

@tadelesh tadelesh force-pushed the tag_and_release_pipeline branch from bc43f99 to 6737356 Compare September 3, 2021 03:08
@tadelesh tadelesh merged commit 694c7a9 into Azure:main Sep 3, 2021
@tadelesh tadelesh deleted the tag_and_release_pipeline branch September 3, 2021 04:43
jay-most pushed a commit to jay-most/azure-sdk-for-go that referenced this pull request Sep 18, 2021
* add manual tag and release pipeline for track2

* fix: get version support data plane

* chore: refine powershell syntax

* feat: use regrex to fetch version from all possible version file under module folders

* fix: polish according to pr review

* fix: remove rewrite CreateReleases function as eng sys has already done it
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.

5 participants