Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions .github/workflows/release-IPA-metrics.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ permissions:
issues: write

jobs:
release-IPA-metrics:
pre-IPA-metrics-release-checks:
name: IPA Metrics Release Pre-Checks
runs-on: ubuntu-latest
outputs:
should_run_release: ${{ steps.get_previous_status.outputs.result }}
steps:
- name: Checkout
uses: actions/checkout@v4
Expand All @@ -39,43 +42,46 @@ jobs:
with:
github-token: ${{ secrets.api_bot_pat }}
script: |
const script = require('tools/spectral/ipa/metrics/scripts/getShouldRunMetricsRelease.js')
const shouldRunRelease = await getShouldRunMetricsRelease({github, context})
const { default: getShouldRunMetricsRelease } = await import('${{ github.workspace }}/tools/spectral/ipa/metrics/scripts/getShouldRunMetricsRelease.js')
const shouldRunRelease = await getShouldRunMetricsRelease({github, context}).catch((error) => {
console.error(error.message);
process.exit(1)
})
return shouldRunRelease

- name: Skip Metric Collection Job
if: ${{steps.get_previous_status.outputs.result == 'false' }}
run: echo "Skipping IPA metrics release!"
release-IPA-metrics:
name: Release IPA Validation Metrics
needs: [pre-IPA-metrics-release-checks]
if: ${{ needs.pre-IPA-metrics-release-checks.outputs.should_run_release == 'true' }}
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup Node
if: ${{steps.get_previous_status.outputs.result == 'true' }}
uses: actions/setup-node@v4
with:
node-version: '20.x'
cache: 'npm'

- name: Install npm dependencies
if: ${{steps.get_previous_status.outputs.result == 'true' }}
run: npm install

- name: Download openapi-foas
if: ${{steps.get_previous_status.outputs.result == 'true' }}
uses: actions/download-artifact@v4
with:
name: openapi-foas-dev # TODO: Change to passed input env
github-token: ${{ secrets.api_bot_pat }}
run-id: ${{ github.run_id }}
# - name: Download openapi-foas
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: What is the reason for commenting them out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that when running the workflow independently, the foas isn't generated and uploaded first, so it can't be found here, so for now I'm using the v2 spec in the repo, but when moving the workflow into the release oas workflow, we can use this again as the foas is generated here in an earlier step

# uses: actions/download-artifact@v4
# with:
# name: openapi-foas-dev # TODO: Change to passed input env
# github-token: ${{ secrets.api_bot_pat }}
# run-id: ${{ github.run_id }}

- name: Run Metric Collection Job
if: ${{steps.get_previous_status.outputs.result == 'true' }}
working-directory: ./tools/spectral/ipa/metrics/scripts
run: node runMetricCollection.js ../../../../../openapi-foas.json
run: node runMetricCollection.js ../../../../../openapi/v2.json # TODO: Change to foas from above

- name: Dump Metric Collection Job Data to S3
if: ${{steps.get_previous_status.outputs.result == 'true' }}
env:
AWS_ACCESS_KEY_ID: ${{ secrets.IPA_S3_BUCKET_DW_STAGING_USERNAME }} # TODO: Change to passed secret
AWS_SECRET_ACCESS_KEY: ${{ secrets.IPA_S3_BUCKET_DW_STAGING_PASSWORD }} # TODO: Change to passed secret
S3_BUCKET_PREFIX: ${{ secrets.IPA_S3_BUCKET_DW_STAGING_PREFIX }} # TODO: Change to passed secret
working-directory: ./tools/spectral/ipa/metrics/scripts
run: node dataDump.js ../../../../../openapi-foas.json
run: node dataDump.js
34 changes: 23 additions & 11 deletions tools/spectral/ipa/metrics/metricS3Upload.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,33 @@ import { getS3Client, getS3FilePath } from './utils/dataDumpUtils.js';
* @param filePath file path to the metrics collection results, uses config.js by default
*/
export async function uploadMetricCollectionDataToS3(filePath = config.defaultMetricCollectionResultsFilePath) {
const client = getS3Client();
const formattedDate = new Date().toISOString().split('T')[0];
console.log('Loading metrics collection data from', filePath);
const metricsCollectionData = JSON.parse(fs.readFileSync(filePath, 'utf8'));
const table = tableFromJSON(metricsCollectionData);
if (metricsCollectionData === undefined || metricsCollectionData.length === 0) {
throw new Error('Loaded metrics collection data is empty');
}

const s3fileProps = getS3FilePath();
const command = new PutObjectCommand({
Bucket: s3fileProps.bucketName,
Key: path.join(s3fileProps.key, formattedDate, 'metric-collection-results.parquet'),
Body: tableToIPC(table, 'stream'),
});
const table = tableFromJSON(metricsCollectionData);
if (table === undefined) {
throw new Error('Unable to transform metrics collection data to table');
}

try {
const response = await client.send(command);
console.log(response);
console.log('Creating S3 Client...');
const client = getS3Client();

console.log('Getting S3 file path...');
const s3fileProps = getS3FilePath();
const formattedDate = new Date().toISOString().split('T')[0];

const command = new PutObjectCommand({
Bucket: s3fileProps.bucketName,
Key: path.join(s3fileProps.key, formattedDate, 'metric-collection-results.parquet'),
Body: tableToIPC(table, 'stream'),
});

console.log('Dumping data to S3...');
return await client.send(command);
} catch (caught) {
if (caught instanceof S3ServiceException && caught.name === 'EntityTooLarge') {
console.error(
Expand Down
14 changes: 11 additions & 3 deletions tools/spectral/ipa/metrics/scripts/dataDump.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ import { uploadMetricCollectionDataToS3 } from '../metricS3Upload.js';
const args = process.argv.slice(2);
const filePath = args[0];

uploadMetricCollectionDataToS3(filePath)
.then(() => console.log('Data dump to S3 completed successfully.'))
.catch((error) => console.error(error.message));
const response = await uploadMetricCollectionDataToS3(filePath).catch((error) => {
console.error(error.message);
process.exit(1);
});

if (!response) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: This if check here might be redundant. I could not think of any case uploadMetricCollectionDataToS3 will return null or undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this check because if the table creation fails due to the json not being iterable, the function will just return undefined (I think it's something with the library, imo it should error but it doesn't), I had this issue before when passing the wrong file as the collection data, which just resulted in undefined being returned, but the script should fail in this instance, so it's an extra check to make sure we actually get some result in the end

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this code block does not return an error somehow, am I right?

const table = tableFromJSON(metricsCollectionData);
  if (table === undefined) {
    throw new Error('Unable to transform metrics collection data to table');
  }

Sure, just wanted to be sure if it is not redundant 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah at least not for this particular case, adding these extra checks just as a precaution in case something isn't working as expected 👍

console.error('PutObject response is undefined');
process.exit(1);
}

console.log('Data dump to S3 completed successfully.');
Original file line number Diff line number Diff line change
@@ -1,27 +1,27 @@
// Used in .github/workflows/release-IPA-metrics.yml
export default async function getShouldRunMetricsRelease({ github, context }) {
const response = await github.actions.listWorkflowRuns({
const response = await github.rest.actions.listWorkflowRuns({
owner: context.repo.owner,
repo: context.repo.repo,
workflow_id: context.workflow,
workflow_id: 'release-IPA-metrics.yml',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Why do we need to specify the workflow name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I am asking: when it starts working as a part of Release job, do we need to change this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When fetching the previous runs from the api, you use the workflow name or ID to specify which workflow you want to get the previous runs for

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, it is not enough to get the workflow from context but we should give the name explicitly. Good find 👍

per_page: 2,
page: 1,
});

if (response === undefined) {
return true;
if (!response || !response.data) {
throw Error('listWorkFlowRuns response is empty');
}

const { data: runs } = response;
const { workflow_runs: runs } = response.data;

if (runs === undefined || runs.length === 0) {
return true;
throw Error('response.data.workflow_runs is empty');
}

const previousStatus = runs[1].status;
const previousResult = runs[1].conclusion;

const lastRunDate = new Date(runs[1].created_at);
const today = new Date();

return previousStatus === 'failure' || today.toDateString() !== lastRunDate.toDateString();
return previousResult === 'failure' || today.toDateString() !== lastRunDate.toDateString();
}
23 changes: 22 additions & 1 deletion tools/spectral/ipa/metrics/scripts/runMetricCollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,28 @@ if (!fs.existsSync(config.defaultOutputsDir)) {
console.log(`Output directory created successfully`);
}

const result = spawnSync('spectral', ['lint', config.defaultOasFilePath, '--ruleset', config.defaultRulesetFilePath]);
if (!fs.existsSync(config.defaultRulesetFilePath)) {
console.error('Could not find ruleset file path', config.defaultRulesetFilePath);
process.exit(1);
}

if (!oasFilePath && !fs.existsSync(config.defaultOasFilePath)) {
console.error('Could not find default OAS file path', config.defaultOasFilePath);
process.exit(1);
}

if (oasFilePath && !fs.existsSync(oasFilePath)) {
console.error('Could not find OAS file path', oasFilePath);
process.exit(1);
}

const result = spawnSync('npx', [
'spectral',
'lint',
oasFilePath ? oasFilePath : config.defaultOasFilePath,
'--ruleset',
config.defaultRulesetFilePath,
]);

if (result.error) {
console.error('Error running Spectral lint:', result.error);
Expand Down
2 changes: 2 additions & 0 deletions tools/spectral/ipa/metrics/utils/dataDumpUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import dotenv from 'dotenv';
import { S3Client } from '@aws-sdk/client-s3';

function loadS3Config() {
console.log('Loading S3 config...');

if (existsSync('.env') && !process.env.S3_BUCKET_PREFIX) {
dotenv.config();
}
Expand Down
Loading