CLOUDP-290416: Fix and refactor IPA metrics release workflow #387
Conversation
lovisaberggren
changed the title
| 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 |
There was a problem hiding this comment.
Q: What is the reason for commenting them out?
There was a problem hiding this comment.
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
| process.exit(1); | ||
| }); | ||
|
|
||
| if (!response) { |
There was a problem hiding this comment.
Q: This if check here might be redundant. I could not think of any case uploadMetricCollectionDataToS3 will return null or undefined.
There was a problem hiding this comment.
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.
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 👍
There was a problem hiding this comment.
Yeah at least not for this particular case, adding these extra checks just as a precaution in case something isn't working as expected 👍
| 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.
Q: Why do we need to specify the workflow name?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
So, it is not enough to get the workflow from context but we should give the name explicitly. Good find 👍
2 participants
Proposed changes
Did some fixes of the IPA release workflow, added some extra error handling and logging for easier troubleshooting, and some refactoring, including splitting
release-IPA-metrics.ymlinto two jobs, one for checking if the release should run, and the second for doing the release (skipped depending on the first step).Tested the run manually on this branch, and it's passing: https://github.com/mongodb/openapi/actions/runs/12934985049
Next steps
release-IPA-metrics.ymlrelease-IPA-metrics.ymlinstead of local v2.jsonLater on, when we confirmed that it works as expected with the OAS release workflow, we'll change to start using the prod S3 bucket.
Jira ticket: CLOUDP-290416