-
Notifications
You must be signed in to change notification settings - Fork 862
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
APIView Creation from PR for Go #24000
base: main
Are you sure you want to change the base?
Conversation
chidozieononiwu
commented
Jan 23, 2025
- This adds a step to publish .gosource artifact and trigger call to APIView endpoint for detecting API changes
47fbccc
to
127671c
Compare
/azp run go - aztemplate |
Azure Pipelines successfully started running 1 pipeline(s). |
115c5b6
to
4461d43
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
4461d43
to
1cc54b9
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -13,6 +13,7 @@ class PackageProps { | |||
[string]$SdkType | |||
[boolean]$IsNewSdk | |||
[string]$ArtifactName | |||
[string]$ModuleName |
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.
If we can avoid it, I would prefer we not add a new property. I know other languages use ArtifactName for different things and it could potentially be used for module name as well.
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.
I can remove this as it turned out I don't even need it here. Go is not setting ArtifactName but instead Module name is added as NotePropertyName
but it does not show up whren the properties are written to the PackageInfo
file. Anyways the name property is sufficient for what I need to accomplish right now.
1cc54b9
to
1caefd1
Compare
eng/scripts/Language-Settings.ps1
Outdated
@@ -58,7 +58,7 @@ function Get-GoModuleProperties($goModPath) | |||
$pkgProp.SdkType = $sdkType | |||
|
|||
$pkgProp | Add-Member -NotePropertyName "VersionFile" -NotePropertyValue $versionFile | |||
$pkgProp | Add-Member -NotePropertyName "ModuleName" -NotePropertyValue $modName | |||
$pkgProp.ModuleName = $modName |
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.
It looks like we use ArtifactName for the modPath and we add to ModName. Can we actually access ModuleName without it being on the base type?
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.
It can be accessed. But in the case where it is written to a file the property does not show up.
c9679c9
to
d7d3aa0
Compare
d7d3aa0
to
86b795c
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
70fb42e
to
02446d7
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
02446d7
to
ff7bf90
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
ff7bf90
to
a5290c1
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
a5290c1
to
5577a33
Compare
/azp run go - aztemplate - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
22e46a3
to
56c2dae
Compare
56c2dae
to
a77e679
Compare
@@ -101,13 +101,11 @@ function Get-AllPackageInfoFromRepo($serviceDirectory) | |||
$searchPath = Join-Path $RepoRoot "sdk" | |||
$pkgFiles = @() | |||
if ($serviceDirectory) { | |||
$searchPath = Join-Path $searchPath $serviceDirectory "go.mod" |
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.
Previous code was not doing recursive search when service directory is passed. I think Go has sub modules which contains go.mod and that's probably the reason we don't do Recurse to find package in service directory.
@benbp Can you please confirm if sub module also can have go.mod?
.PARAMETER DirectoryToPublish | ||
Directory containing all artifacts to be publisehd to the pipeline | ||
#> | ||
function New-GoArtifacts { |
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.
Can you please rename this function to Create-ApiView-Artifact
to be more specific. Go does not have any artifact and this generic name will be misleading.
foreach ($sdk in (Get-AllPackageInfoFromRepo $ServiceDirectory)) | ||
{ | ||
Write-Host "Creating API review artifact for $($sdk.Name)" | ||
$sdkDirectoryPath = Join-Path -Path $OutputDirectory $sdk.Name |
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.
What's the value of sdk.name? Go has same module name under different path and that's the reason it uses complete sdk path as package name. We cannot use just the name of last folder that contains go.mod to create unique package folders.
@@ -0,0 +1,100 @@ | |||
Write-Host "$PSScriptRoot" |
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.
Can also please rename this script to apiview-helpers.ps1
?
-DirectoryToPublish $directoryToPublish | ||
|
||
Copy-Item "$(Build.ArtifactStagingDirectory)/PackageInfo" -Destination $directoryToPublish -Recurse | ||
displayName: 'Create Go Package Artifact' |
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.
displayName: 'Create Go Package Artifact' | |
displayName: 'Create Go APIView Artifact' |
- template: /eng/common/pipelines/templates/steps/publish-1es-artifact.yml | ||
parameters: | ||
ArtifactName: "PackageInfo" |
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.
Don't we still need old PackageInfo for other EngSys tools?