-
Notifications
You must be signed in to change notification settings - Fork 15
CLOUDP-290416: Add workflow for releasing IPA metrics #386
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
Conversation
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Get Previous Run Date and Status |
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.
Only run the release steps if the latest run was not today or if the last run ended in failure
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.
Could you clarify this decision? Adding a comment on the step would be beneficial to remember why we made this decision.
Why can't we run the workflow once a day automatically and file an issue on the repo in the event of a failure?
| - name: Run Metric Collection Job | ||
| if: ${{steps.get_previous_status.outputs.result == 'true' }} | ||
| working-directory: ./tools/spectral/ipa/metrics/scripts | ||
| run: node ./tools/spectral/ipa/metrics/scripts/runMetricCollection.js ../../../../../openapi-foas.json |
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.
Q1: Why don't we use the downloaded openapi-foas from the previous step?
Q2: If we set the working directory, do we need to give the full path for the script file?
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.
Q1: It should use the downloaded foas, the downloaded one is named openapi-foas.yaml
Q2: Good catch, did a change and forgot to change the command, it should be node runMetricCollection.js ../../../../../openapi-foas.json, I'll update!
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 assumed it would be named something like openapi-foas-dev. Thanks for the info 👍
| run: node runMetricCollection.js ../../../../../openapi-foas.json | ||
|
|
||
| - name: Dump Metric Collection Job Data to S3 | ||
| if: ${{steps.get_previous_status.outputs.result == 'true' }} |
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.
Nit: we might use working-directory here as well
|
|
||
| const lastRunDate = new Date(runs[1].created_at); | ||
| const today = new Date(); | ||
| const lastRunWasToday = |
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.
Nit: you can check if two dates are equal with checking if their toDateString() equal to each other
if(lastRunDate.toDateString() === today.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.
I think this will also include hours, minutes and seconds, which may not be the same, so opted for only checking the day
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.
Nope, it only outputs the day, month, and year (link)
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.
Ah nice, missed toDateString! I'll update
yelizhenden-mdb
left a comment
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.
LGTM! Exciting! 🚀
| @@ -0,0 +1,81 @@ | |||
| name: IPA Validation Metrics Release | |||
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.
Could you add a comment at the top of this file explaining what the workflow is doing? What metrics release mean? where are they stored? By looking at the file is seems that it pushing something to s3 maybe but it is not really clear 🤔
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.
Sure! I can add some comments to clarify
| # aws_access_key: | ||
| # required: true | ||
| # aws_secret_key: | ||
| # required: true | ||
| # aws_s3_bucket_prefix: | ||
| # required: true |
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.
why do you have these comments? Can we remove them?
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.
These have been removed in this PR: https://github.com/mongodb/openapi/pull/388/files Where it's also included in the oas release workflow
| # inputs: | ||
| # env: | ||
| # description: 'Environment for the FOAS to use for IPA metrics collection' | ||
| # required: true | ||
| # type: string |
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.
same here
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.
Removed in this PR: https://github.com/mongodb/openapi/pull/388/files
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Get Previous Run Date and Status |
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.
Could you clarify this decision? Adding a comment on the step would be beneficial to remember why we made this decision.
Why can't we run the workflow once a day automatically and file an issue on the repo in the event of a failure?
| 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 |
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.
Can the remove the todo comments?
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.
Removed in this PR: https://github.com/mongodb/openapi/pull/388/files
This was a request on the scope, to add a check before uploading the IPA metrics so that:
I can add some comments to clarify what it does
We wanted to run the workflow as part of the oas release, so that it's triggered only if there are changes detected (similarly to postman release), currently this is covered in the follow up PR which also includes IPA release in the failure-handler |
Proposed changes
Adding workflow to release IPA metrics to s3.
Follow up:
release-spec.ymlrun when we're confident it works as expectedfailure-handler.ymlto also consider ipa release run result statusJira ticket: CLOUDP-290416