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

Integrate Armstrong Validation into the spec PR check. #28829

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

Conversation

ms-zhenhua
Copy link
Contributor

Migrate from #28803

Copy link

openapi-pipeline-app bot commented Apr 24, 2024

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

Copy link

openapi-pipeline-app bot commented Apr 24, 2024

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️LintDiff succeeded [Detail] [Expand]
Validation passes for LintDiff.
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented Apr 24, 2024

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app bot commented Apr 24, 2024

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

function Validate-Terraform-Error($repoPath, $filePath) {
$fileDirectory = (Split-Path -Parent $filePath)

$outputDirectory = Join-Path -Path $fileDirectory -ChildPath "58d50903-36e9-4f57-a1e5-f246d7ecdec0"
Copy link
Member

@mikeharder mikeharder Apr 25, 2024

Choose a reason for hiding this comment

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

You should create a unique temp directory using code like this:

Suggested change
$outputDirectory = Join-Path -Path $fileDirectory -ChildPath "58d50903-36e9-4f57-a1e5-f246d7ecdec0"
$outputDirectory= [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), [System.IO.Path]::GetRandomFileName())
try {
New-Item -Path $outputDirectory -ItemType Directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated. Thanks.

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Improve code to generate and cleanup temp folder

vmImage: ubuntu-22.04

# refer to https://github.com/MicrosoftDocs/pipelines-go/blob/main/azure-pipelines.yml
variables:
Copy link
Member

Choose a reason for hiding this comment

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

Could the pipeline work without setting these vars? Because these would also need to be different on Windows vs Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@ms-zhenhua ms-zhenhua force-pushed the ms-zhenhua/armstrong-validation branch from 333fb0e to da9dda0 Compare April 26, 2024 05:30
@ms-zhenhua ms-zhenhua force-pushed the ms-zhenhua/armstrong-validation branch from eda376f to 440c631 Compare April 28, 2024 23:18
Comment on lines +117 to +122
$outputDirectory = [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), [System.IO.Path]::GetRandomFileName())
$result = @()

try {
if (!(Test-Path -Path $outputDirectory)) {
New-Item -Path $outputDirectory -ItemType Directory *> $null
Copy link
Member

@mikeharder mikeharder May 1, 2024

Choose a reason for hiding this comment

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

Suggested change
$outputDirectory = [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), [System.IO.Path]::GetRandomFileName())
$result = @()
try {
if (!(Test-Path -Path $outputDirectory)) {
New-Item -Path $outputDirectory -ItemType Directory *> $null
$outputDirectory = [System.IO.Directory]::CreateTempSubdirectory()
$result = @()
try {

.NET 7 added an easier way to do this.

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Use simpler API to create temp directory


# -NoEnumerate to prevent single-element arrays from being collapsed to a single object
# -AsHashtable is closer to raw JSON than PSCustomObject
$suppressions = npx get-suppressions ArmstrongValidation $fileInSpecFolder | ConvertFrom-Json -NoEnumerate -AsHashtable
Copy link
Member

@mikeharder mikeharder May 7, 2024

Choose a reason for hiding this comment

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

@ms-zhenhua: I updated your PR to use the new get-suppressions shared code added in #28927, #29001, and #29008.

You may also want to add parameter $CheckAllUnder to your script for easier testing, like this:

https://github.com/Azure/azure-rest-api-specs/pull/28927/files#diff-3c37fefe028ad9bd154b76ea3371f8db2d7a7609dafa323e92d8461880e808b2R9

@AzureRestAPISpecReview AzureRestAPISpecReview removed the VersioningReviewRequired <valid label in PR review process>add this label when versioning review is required label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants