-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Allow proper setting of CI-RpaaSRPNotInPrivateRepo
#36405
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to use consistent names for all 3 related files in this check. How about "RPaaS Branch Validation"?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add tests. Refactor into exported functions if it helps. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,135 @@ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import { getChangedFiles, getChangedFilesStatuses, swagger } from "../src/changed-files.js"; | ||||||||||||||||||||||||||||||||||||||
|
Check failure on line 2 in .github/shared/cmd/detect-missing-rpaas-rp.js
|
||||||||||||||||||||||||||||||||||||||
| import { parseArgs } from "node:util"; | ||||||||||||||||||||||||||||||||||||||
| import { dirname } from "node:path"; | ||||||||||||||||||||||||||||||||||||||
| import { fileURLToPath } from "node:url"; | ||||||||||||||||||||||||||||||||||||||
| import { readdir, access, stat } from "node:fs/promises"; | ||||||||||||||||||||||||||||||||||||||
| import { join } from "node:path"; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| function usage() { | ||||||||||||||||||||||||||||||||||||||
| console.log(`Usage: | ||||||||||||||||||||||||||||||||||||||
| npx api-doc-preview --output <output-dir> | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update to match new name and params |
||||||||||||||||||||||||||||||||||||||
| parameters: | ||||||||||||||||||||||||||||||||||||||
| --repoWithChanges <repo directory> Directory checked out with PR Changes | ||||||||||||||||||||||||||||||||||||||
| --referenceFolder <azure-rest-api-specs-pr@RPSaaSMAster> The azure-rest-api-specs-pr directory checked out at RPSaaSMaster branch.`); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const { | ||||||||||||||||||||||||||||||||||||||
| values: { | ||||||||||||||||||||||||||||||||||||||
| "repoWithChanges": repoWithChanges, | ||||||||||||||||||||||||||||||||||||||
| "referenceFolder": referenceFolder | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| } = parseArgs({ | ||||||||||||||||||||||||||||||||||||||
| options: { | ||||||||||||||||||||||||||||||||||||||
| "repoWithChanges": { | ||||||||||||||||||||||||||||||||||||||
| type: "string", | ||||||||||||||||||||||||||||||||||||||
| default: process.env.BUILD_SOURCESDIRECTORY || "", | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| "referenceFolder": { | ||||||||||||||||||||||||||||||||||||||
| type: "string", | ||||||||||||||||||||||||||||||||||||||
| default: process.env.PRIVATESPECSDIRECTORY || "", | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| allowPositionals: false, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let validArgs = true; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (!repoWithChanges) { | ||||||||||||||||||||||||||||||||||||||
| console.log( | ||||||||||||||||||||||||||||||||||||||
| `Missing required parameter --repoWithChanges. Value given: ${repoWithChanges || "<empty>"}`, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| validArgs = false; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (!referenceFolder) { | ||||||||||||||||||||||||||||||||||||||
| console.log( | ||||||||||||||||||||||||||||||||||||||
| `Missing required parameter --referenceFolder. Value given: ${referenceFolder || "<empty>"}`, | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| validArgs = false; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (!validArgs) { | ||||||||||||||||||||||||||||||||||||||
| usage(); | ||||||||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const changedFiles = await getChangedFiles({ | ||||||||||||||||||||||||||||||||||||||
| cwd: repoWithChanges, | ||||||||||||||||||||||||||||||||||||||
| paths: ["specification"], | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Extract RP folders from changed files | ||||||||||||||||||||||||||||||||||||||
| // Files will be in format: specification/<rp>/<rest-of-path> | ||||||||||||||||||||||||||||||||||||||
| const changedRpFolders = new Set(); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| for (const filePath of changedFiles) { | ||||||||||||||||||||||||||||||||||||||
| // Split the path and check if it starts with 'specification' | ||||||||||||||||||||||||||||||||||||||
| const pathParts = filePath.split('/'); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (pathParts.length >= 2 && pathParts[0] === 'specification') { | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+69
to
+72
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We know it must start with |
||||||||||||||||||||||||||||||||||||||
| // The second part is the RP folder name | ||||||||||||||||||||||||||||||||||||||
| const rpFolder = pathParts[1]; | ||||||||||||||||||||||||||||||||||||||
| changedRpFolders.add(rpFolder); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| console.log(`Found ${changedRpFolders.size} changed RP folders:`, Array.from(changedRpFolders)); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Get the list of RP folders from the reference directory | ||||||||||||||||||||||||||||||||||||||
| const referenceSpecPath = join(referenceFolder, 'specification'); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Check if reference directory exists | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| await access(referenceSpecPath); | ||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||
| console.error(`Reference specification directory does not exist: ${referenceSpecPath}`); | ||||||||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+84
to
+90
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
should be sufficient to let FS error bubble and fail app |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let referenceRpFolders; | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| // Get all items in the reference specification folder | ||||||||||||||||||||||||||||||||||||||
| const items = await readdir(referenceSpecPath); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Filter to only directories | ||||||||||||||||||||||||||||||||||||||
| referenceRpFolders = []; | ||||||||||||||||||||||||||||||||||||||
| for (const item of items) { | ||||||||||||||||||||||||||||||||||||||
| const itemPath = join(referenceSpecPath, item); | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| const stats = await stat(itemPath); | ||||||||||||||||||||||||||||||||||||||
| if (stats.isDirectory()) { | ||||||||||||||||||||||||||||||||||||||
| referenceRpFolders.push(item); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } catch (statError) { | ||||||||||||||||||||||||||||||||||||||
| // Skip items we can't stat (permissions, etc.) | ||||||||||||||||||||||||||||||||||||||
| console.warn(`Warning: Could not stat ${itemPath}:`, statError.message); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+95
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||
| console.error(`Error reading reference directory ${referenceSpecPath}:`, error.message); | ||||||||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+111
to
+114
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
sufficient to let exception bubble |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| console.log(`Found ${referenceRpFolders.length} RP folders in reference directory`); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Check if all changed RP folders exist in the reference directory | ||||||||||||||||||||||||||||||||||||||
| const missingRpFolders = []; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| for (const rpFolder of changedRpFolders) { | ||||||||||||||||||||||||||||||||||||||
| if (!referenceRpFolders.includes(rpFolder)) { | ||||||||||||||||||||||||||||||||||||||
| missingRpFolders.push(rpFolder); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (missingRpFolders.length > 0) { | ||||||||||||||||||||||||||||||||||||||
| console.error(`ERROR: The following RP folders are missing from the reference directory:`); | ||||||||||||||||||||||||||||||||||||||
| for (const missingFolder of missingRpFolders) { | ||||||||||||||||||||||||||||||||||||||
| console.error(` - ${missingFolder}`); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| console.error(`These folders must exist in the reference directory before changes can be processed.`); | ||||||||||||||||||||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| process.exit(0); | ||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going to ask if this should be a separate WF, or part of summarize-checks? But it looks like summarize_checks only triggers on WF run, not check run, and we probably don't want to change that? So we need an intermediate WF to bridge the gap with devops? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| name: "RPaaS in Private Check" | ||
|
|
||
| on: | ||
| check_run: | ||
| types: [completed] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| checks: read | ||
|
|
||
| jobs: | ||
| rpaas-in-private-check: | ||
| if: | | ||
| github.event.check_run.check_suite.app.name == 'Azure Pipelines' && | ||
| contains(github.event.check_run.name, 'PresentInRPaaSCheck') | ||
| name: "RPaaS In Private" | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| sparse-checkout: | | ||
| .github | ||
| # todo: how much is this stuff actually necessary for our rpass-private-check-status? | ||
| # Only run the login and get token steps when the respository is azure-rest-api-specs-pr | ||
| - if: github.event.repository.name == 'azure-rest-api-specs-pr' | ||
| name: Azure Login with Workload Identity Federation | ||
| uses: azure/login@v2 | ||
| with: | ||
| client-id: "936c56f0-298b-467f-b702-3ad5bf4b15c1" | ||
| tenant-id: "72f988bf-86f1-41af-91ab-2d7cd011db47" | ||
| allow-no-subscriptions: true | ||
|
|
||
| - if: github.event.repository.name == 'azure-rest-api-specs-pr' | ||
| name: Get ADO Token via Managed Identity | ||
| run: | | ||
| # Get token for Azure DevOps resource | ||
| ADO_TOKEN=$(az account get-access-token --resource "499b84ac-1321-427f-aa17-267ca6975798" --query "accessToken" -o tsv) | ||
| echo "ADO_TOKEN=$ADO_TOKEN" >> "$GITHUB_ENV" | ||
| - name: "Get Issue Number" | ||
| id: get-issue-number | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const { extractInputs } = | ||
| await import('${{ github.workspace }}/.github/workflows/src/context.js'); | ||
| const { issue_number } = await extractInputs({ github, context, core }); | ||
| core.setOutput("issue_number", issue_number); | ||
|
Comment on lines
+41
to
+50
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will move this to a shared GH Action after you merge.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| - if: ${{ always() && steps.get-issue-number.outputs.issue_number }} | ||
| name: Upload artifact with issue number | ||
| uses: ./.github/actions/add-empty-artifact | ||
| with: | ||
| name: issue-number | ||
| value: ${{ steps.get-issue-number.outputs.issue_number }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| name: "[TEST-IGNORE] Summarize Checks" | ||
|
|
||
| on: | ||
|
|
@@ -9,6 +9,7 @@ | |
| - "Swagger Avocado - Set Status" | ||
| - "Swagger LintDiff - Set Status" | ||
| - "SDK Validation Status" | ||
| - "RPaaS in Private Check" | ||
| types: | ||
| - completed | ||
| pull_request_target: # when a PR is labeled. NOT pull_request, because that would run on the PR branch, not the base branch. | ||
|
|
@@ -55,8 +56,8 @@ | |
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const { default: dumpTriggerMetadata } = | ||
| await import('${{ github.workspace }}/.github/workflows/src/summarize-checks/dump-trigger-metadata.js'); | ||
| const { extractInputs } = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bug? |
||
| await import('${{ github.workspace }}/.github/workflows/src/context.js'); | ||
| return await dumpTriggerMetadata({ context, core }); | ||
|
|
||
| - id: summarize-checks | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||||||
| trigger: none | ||||||||||||||
| pr: | ||||||||||||||
| paths: | ||||||||||||||
| include: | ||||||||||||||
| # Trigger for files that will result in a specification change | ||||||||||||||
| - specification/** | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Could we limit to only PRs impacting swaggers under the resource-manager folder? |
||||||||||||||
|
|
||||||||||||||
| jobs: | ||||||||||||||
| - job: PresentInRPaaSCheck | ||||||||||||||
| name: Check RP Presenence in RPSaaSMaster | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| pool: | ||||||||||||||
| name: $(LINUXPOOL) | ||||||||||||||
| vmImage: $(LINUXVMIMAGE) | ||||||||||||||
|
|
||||||||||||||
| variables: | ||||||||||||||
| - template: /eng/pipelines/templates/variables/globals.yml | ||||||||||||||
| - template: /eng/pipelines/templates/variables/image.yml | ||||||||||||||
|
|
||||||||||||||
| - name: PrivateSpecsDirectory | ||||||||||||||
| value: $(Pipeline.Workspace)/azure-rest-api-specs-pr | ||||||||||||||
|
|
||||||||||||||
| steps: | ||||||||||||||
| - checkout: self | ||||||||||||||
| # Fetch depth required to get list of changed files | ||||||||||||||
| fetchDepth: 2 | ||||||||||||||
|
|
||||||||||||||
| - template: /eng/common/pipelines/templates/steps/sparse-checkout.yml | ||||||||||||||
| parameters: | ||||||||||||||
| SkipCheckoutNone: true | ||||||||||||||
| # Path does not need to be set because sparse-checkout.yml already | ||||||||||||||
| # checks out files in the repo root | ||||||||||||||
| Repositories: | ||||||||||||||
| - Name: Azure/azure-rest-api-specs-pr | ||||||||||||||
| Commitish: RPSaaSMaster | ||||||||||||||
| WorkingDirectory: $(PrivateSpecsDirectory) | ||||||||||||||
|
|
||||||||||||||
| - template: /eng/pipelines/templates/steps/npm-install.yml | ||||||||||||||
| parameters: | ||||||||||||||
| WorkingDirectory: .github/shared | ||||||||||||||
|
Comment on lines
+38
to
+40
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We could improve perf with a custom install command if needed ( |
||||||||||||||
|
|
||||||||||||||
| - script: .github/shared/cmd/detect-missing-rpaas-rp.js | ||||||||||||||
| displayName: Compare RP Folder Usage | ||||||||||||||
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.
Move to
.github/workflows/cmd