-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-298521: Decouple IPA Metrics Collection Job from FOAS Release GH Workflow #401
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
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20.x' | ||
| node-version: $NODE_VERSION |
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.
FYI: It is possible to reference local variables this way (https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#defining-environment-variables-for-a-single-workflow)
| failure-handler: | ||
| name: Failure Handler | ||
| needs: [ release-IPA-metrics ] | ||
| if: ${{ 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.
| openapi/v2/openapi*.json | ||
| changelog/changelog.json | ||
| changelog/internal/changelog-all.json | ||
| changelog/internal/metadata.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.
I think we don't need to upload the changelog files here, since we're only using openapi files
| required: true | ||
| type: string | ||
| schedule: | ||
| - cron: '0 22 * * *' # Runs daily at 22:00 UTC (10 PM UTC) |
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.
What are the reasons to run it outside working hours? Keep in mind that you will get a notification if the action fails
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.
Good point. Changed it to 11:00 AM. Do we have any standards around it?
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.
Related: Perhaps we should only run mon-fri as well? The OASes don't get updated during the weekend either AFAIK
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.
Do we have any standards around it?
No, it is up to the team. Ideally, it should be working hours so that we can action on 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.
Related: Perhaps we should only run mon-fri as well? The OASes don't get updated during the weekend either AFAIK
Yes, but the metric collection job is not related to OAS updates anymore. And, DPE team asked us to send the data every day if possible. I checked with them about skipping weekend, but they need the data to be sent every 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.
LGTM. Great work on this 👍
| OAS_ENV: 'dev' | ||
| OAS_BRANCH: 'dev' |
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.
Since they are the same, you just need one right?
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.
Yes, the OAS_ENV is used for the failure handler, but I believe it also represents the environment where the OpenAPI specification is generated, so I can combine them into one variable 👍
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.
🚀
Proposed changes
Jira ticket: CLOUDP-298521
Checklist
Changes to Spectral