Azure / Azure/azure-rest-api-specs

Integrate Armstrong Validation into the spec PR check suite.

Open
#28,773 9 comments 0 reactions 2 assignees Claimed by @ms-zhenhua View on GitHub
Central-EngSys Spec PR Tools
Dominant language
TypeSpec
Stars
3.1k
Forks
5.9k
Avg merge
2d 22h
Merged PRs (30d)
444

Description

Our partner @ms-zhenhua is working on adding a new check to the spec PR check suite.

## Overview of the proposed check

The proposed initial check implementation can be currently found in this PR:
- [Pull Request 544843](https://dev.azure.com/devdiv/DevDiv/_git/openapi-alps/pullrequest/544843): Add Armstrong Validation Task

Per that PR description, the proposed check is checking for presence of credentials in Terraform (`.tf`) files by leveraging https://github.com/Azure/armstrong.

## General approach to implementing the check

@ms-zhenhua we no longer add any new checks to the `openapi-alps` repository. All new checks should follow the new check integration model we adopted.

Eventually we will provide a full guide (https://github.com/Azure/azure-rest-api-specs/issues/28772) but for now below I provide some pointers to help you get started.

## Implementing a new check: a quickstart

@ms-zhenhua We have few checks following this model:

- PowerShell-based `TypeSpec Requirement` GitHub check.
- TypeScript-based `TypeSpec Validation` GitHub check.
- `Swagger PrettierCheck` GitHub check migrated from `openapi-alps` to the new model.

You can find sources of all these checks in https://github.com/Azure/azure-rest-api-specs/tree/main/eng.

You should implement your new check by copy-pasting and adapting one of the existing checks. I.e. you should create a PR that will add your new check artifacts inside the https://github.com/Azure/azure-rest-api-specs/tree/main/eng directory. We will review it.

We recommend keeping it simple and implementing it using PowerShell, like `TypeSpec Requirement`. Consider using TypeScript only if you think the check is too complex for PowerShell.

Let me use `TypeSpec Requirement` to explain a bit more how these checks work.

- You can see that `TypeSpec Requirement` is one of the checks running on specs PRs. [Here is a recent example](https://github.com/Azure/azure-rest-api-specs/pull/28634/checks?check_run_id=24047899809).
- We have [`TypeSpec Requirement` ADO pipeline](https://dev.azure.com/azure-sdk/public/_build?definitionId=6659) that powers this check. It is integrated with GitHub by virtue of being based on GitHub repository and using appropriate service connection. See the [YAML tab](https://dev.azure.com/azure-sdk/public/_apps/hub/ms.vss-ciworkflow.build-ci-hub?_a=edit-build-definition&id=6659&view=Tab_Tasks) / `Get sources` sub-tab.
- The pipeline definition is [`typespec-requirement.yml`](https://github.com/Azure/azure-rest-api-specs/blob/main/eng/pipelines/typespec-requirement.yml).
- The pipeline calls into [TypeSpec-Requirement.ps1](https://github.com/Azure/azure-rest-api-specs/blob/main/eng/scripts/TypeSpec-Requirement.ps1) which has the business logic. If it returns `exit 1` then the check will be reported as failed.
- We have configured the relevant branch protection rule to make the `TypeSpec Requirement (resource-manager)` (which is one of the entries in the job matrix in the pipeline definition) `required` so when it fails, it will block the PR.
- Because the check is `required`, it will automatically be included in the `Automated merging requirements met` check.
- To ensure appropriate message is provided in the `Next Steps to Merge` comment if this check fails, we have [the following line of code](https://devdiv.visualstudio.com/DevDiv/_git/openapi-alps?path=/public/swagger-validation-common/src/checksWorkflowInfo.ts&version=GBmain&line=132&lineEnd=132&lineStartColumn=3&lineEndColumn=96&lineStyle=plain&_a=contents): in `openapi-alps`: `createCheckInfo(0, "TypeSpec Requirement (resource-manager)", [], typeSpecRequirementArmTsg),`. That is the only integration point with `openapi-alps`.
- For suppression support, see `Implementing support for suppressions` section below.

When implementing your check, you should mimic the above setup. We will help with code review, setting up relevant branch protection rule and any other questions you have.

## Implementing support for suppressions

@ms-zhenhua re suppressions:

The official guidance on **existing suppressions** is here:
- https://eng.ms/docs/products/azure-developer-experience/design/specs-pr-guides/pr-suppressions

And here is additional guidance for us devs:
- https://github.com/Azure/azure-sdk-tools/issues/8133

The `Approved-***` is obsolete model which we slowly migrate away from. You can read more about this in the design section in https://github.com/Azure/azure-sdk-tools/issues/8133.

Instead, we will want for you to provide suppression via appropriate entry in the [`suppresions.yaml` file](https://eng.ms/docs/products/azure-developer-experience/design/specs-pr-guides/pr-suppressions#suppressionsyaml). The aforementioned [TypeSpec-Requirement.ps1](https://github.com/Azure/azure-rest-api-specs/blob/main/eng/scripts/TypeSpec-Requirement.ps1) implements support for reading `suppressions.yaml`. You can reuse that code for your own check. You can also read about the design here: https://github.com/Azure/azure-rest-api-specs/issues/27348.

## Notes on the existing PR

@ms-zhenhua we did preliminary review of [Pull Request 544843](https://dev.azure.com/devdiv/DevDiv/_git/openapi-alps/pullrequest/544843): Add Armstrong Validation Task.

Our initial conclusions are as follows:

- The code to install "go" and the the "azure/armstrong" package should be moved into pipeline yml.
- The core of the check appears to be following logic which we think can be implemented in PowerShell-based check:
- Find all files changed in PR named "main.tf"
- Run "armstrong credscan" on matching folders
- If any errors, flow to GitHub errors and fail check
- The name appears to be wrong. It should be called `Terraform Armstrong Validation` not `Swagger ArmstrongValidation`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.