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

Descend from the alps/swagger api view #31320

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

chidozieononiwu
Copy link
Member

This adds a job for identifying swagger files in a given pull-request and generate relevant swagger APIView token artifacts.

Copy link

openapi-pipeline-app bot commented Oct 29, 2024

Next Steps to Merge

⌛ Please wait. Next steps to merge this PR are being evaluated by automation. ⌛

Copy link

openapi-pipeline-app bot commented Oct 29, 2024

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

@chidozieononiwu chidozieononiwu self-assigned this Oct 29, 2024
@chidozieononiwu chidozieononiwu marked this pull request as draft October 29, 2024 21:25
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from ad604d4 to 1588bf8 Compare October 29, 2024 21:44
.OUTPUTS
the readme.md file associated with the swagger file or null if not found.
#>
function Get-SwaggerReadMeFile {
Copy link
Member

Choose a reason for hiding this comment

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

How about moving this function and the subsequent one into a common PS script? They can be reused by other CI checks as well.

Copy link
Member

Choose a reason for hiding this comment

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

looks like this could also happen to Get-ResourceProviderFromReadMePath

@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch 3 times, most recently from 00e600e to cc13604 Compare November 6, 2024 22:29
@chidozieononiwu chidozieononiwu marked this pull request as ready for review November 6, 2024 22:29

. "$PSScriptRoot/Logging-Functions.ps1"

$apiViewArtifactsDirectory = [System.IO.Path]::Combine($ArtiFactsStagingDirectory, $APIViewArtifactsDirectoryName)
Copy link
Member

@weshaggard weshaggard Nov 6, 2024

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

That script depends a lot on packageProperties logic which is not implemented for azure-rest-api-spec. I tried to do a refactor but felt it made the script overly complex

Copy link
Member

Choose a reason for hiding this comment

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

It feels like we could consider creating a helper function to do the APIView call itself. That way we can use common invoking and error handling. Even if for now you just use that new function here with the intent to also consume it elsewhere that might be a good step in the right direction.

$query.Add('pullRequestNumber', $PullRequestNumber)
$query.Add('packageName', $_.BaseName)
$query.Add('language', $Language)
$query.Add('commentOnPR', $false)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be true?

@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from 8ba5aa0 to 6c30ecf Compare November 7, 2024 01:11
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from faf9563 to d50a544 Compare November 7, 2024 18:02
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from d50a544 to e886b12 Compare November 7, 2024 18:05
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from 49768b2 to 7273e1f Compare November 7, 2024 18:26
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from c36a1dd to 9c517ef Compare November 7, 2024 22:11
@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from ab3e67c to 946c398 Compare November 7, 2024 22:18
DOTNET_ROOT: /home/runner/.dotnet

- name: Publish Swagger APIView Artifacts
uses: actions/upload-artifact@v3
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
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4

-RepoName ${{ github.repository }} `
-PullRequestNumber ${{ env.PullRequestNumber }} `
-Language 'Swagger' `
-CommitSha ${{ github.sha }}
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
-CommitSha ${{ github.sha }}
-CommitSha ${{ github.sha }}


LogGroupStart " See all generated Swagger APIView Artifacts..."
Get-ChildItem -Path $swaggerAPIViewArtifactsDirectory -Recurse
LogGroupEnd
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
LogGroupEnd
LogGroupEnd

@chidozieononiwu chidozieononiwu force-pushed the descendFromTheAlps/SwaggerAPIView branch from b615d04 to c6788b4 Compare November 7, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔬 Dev in PR
Development

Successfully merging this pull request may close these issues.

6 participants