-
Notifications
You must be signed in to change notification settings - Fork 229
Add release check for open offsets PRs #2680
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
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
.github/workflows/publish-modules.yml:14
- [nitpick] The job name 'verify-offsets-merged' is misleading since it checks for open PRs rather than merged ones. Consider renaming it to 'verify-open-offsets-prs' for improved clarity.
verify-offsets-merged:
run: | | ||
curl -s -H "Accept: application/vnd.github+json" \ | ||
-H "Authorization: Bearer ${{ secrets.RELEASE_BOT_TOKEN }}" \ | ||
"https://api.github.com/repos/odigos-io/${{ matrix.repo }}/pulls?state=open&per_page=100" | \ |
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.
Will this max out at 100 PRs?
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.
Yeah, the max is 100 and the default is 30 https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests
@@ -11,6 +11,20 @@ on: | |||
required: true | |||
|
|||
jobs: | |||
verify-offsets-merged: |
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 need to add this step as a requirement for the next ones, or if it fails the next ones won't get executed? I don't remember if jobs in the same workflow are done in parallel by default (think that they do)
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.
Yeah good point, I added that
## Description Similar to #2680, this adds a pre-release check for open enterprise dependency sync PRs. Currently, the release flow will create a release PR in the enterprise repo that is effectively a dependency sync. However, if there are currently failing tests with the current dependency sync, it would be good to catch these before we start a release flow instead of having to debug it mid release. Making this a separate job so it can depend on the verify-offsets check, which itself will trigger a dependency bump if there are offsets changes to be merged ## How Has This Been Tested? <!-- Describe the tests you ran and how you verified your changes. --> - [ ] Added Unit Tests - [ ] Updated e2e Tests - [ ] Manual Testing - [ ] Manual Load Test ## Kubernetes Checklist <!-- If this PR affects how Odigos interacts with Kubernetes, check the relevant boxes below and provide more details --> - [ ] Changes how Odigos interacts with Kubernetes - [ ] Introduces additional calls to the API Server (potential performance impact) - [ ] New Query/feature supported in all the k8s versions supported by Odigos - [ ] Modifies Odigos manifests (addressed in both CLI and Helm) - [ ] Changes RBAC permissions ## User Facing Changes <!-- Any changes that users will notice or need to be aware of --> - [ ] Users need to take action before upgrading - [ ] Automatic migration will modify existing objects (backward compatible) - [ ] Changes UI, CLI, or K8s Manifests aspects in a way that users need to be aware of - [ ] Documentation updated accordingly
Description
This adds a step at the start of a release to make sure there are no open offsets PRs in our enterprise Go repo. This can be expanded with other languages as well as new labels using the matrix (even though the matrix is currently one value)
How Has This Been Tested?
Kubernetes Checklist
User Facing Changes