-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-290416: Fix and refactor IPA metrics release workflow #387
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
6c940df
5004800
3830d42
f103e92
a9134d9
382e55b
75db32e
acc9b11
74afd2d
a61bc8c
a91cb50
a09402b
7061654
daa9e79
0182e8f
cae5a5c
03356cd
90d2906
964cfa7
f1bce3b
63542b9
026a012
e35631b
ad30f12
8ed16c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
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. Q: This if check here might be redundant. I could not think of any case 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 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 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. So, this code block does not return an error somehow, am I right? Sure, just wanted to be sure if it is not redundant 👍 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. 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', | ||
|
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. Q: Why do we need to specify the workflow name? 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. The reason I am asking: when it starts working as a part of Release job, do we need to change this? 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. 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 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. 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. 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(); | ||
| } | ||
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.
Q: What is the reason for commenting them out?
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.
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