-
Notifications
You must be signed in to change notification settings - Fork 534
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
chore: add release-please automation #659
Conversation
Codecov Report
@@ Coverage Diff @@
## main #659 +/- ##
==========================================
- Coverage 96.68% 95.08% -1.61%
==========================================
Files 13 173 +160
Lines 634 12267 +11633
Branches 124 1172 +1048
==========================================
+ Hits 613 11664 +11051
- Misses 21 603 +582
|
I'll leave this as a draft since it's blocked on #657, which ought to be the final manual, repo-wide release in contrib. But it is ready for review, apart from updating |
"detectors/node/opentelemetry-resource-detector-github": "0.25.0", | ||
"metapackages/auto-instrumentations-node": "0.25.0", | ||
"metapackages/auto-instrumentations-web": "0.25.0", | ||
"packages/opentelemetry-browser-extension-autoinjection": "0.25.0", |
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.
The manifest will be regenerated anyway on the next release so having extra entries is actually not a problem. Missing entries would be a bigger deal.
@dyladan Do we want to also include releasing to NPM & GitHub in the release-please workflow as part of this PR, or should we do that in a separate PR? Reference: https://github.com/googleapis/release-please#automating-publication-to-npm |
@willarmiros whatever is easier for you |
@dyladan I'll just update this PR to include the NPM and GH release steps |
@obecny @vmarchaud I've finished the release-please automation for contrib and tested it as stated in description. PTAL if you have a chance :) |
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
from code perspective it looks fine, but I will defer the review to @dyladan who is more familiar with releasing |
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.
Looks good just some minor clarifications
|
||
# The logic below handles the npm publication: | ||
- name: Checkout Repository | ||
if: ${{ steps.release.outputs.releases_created }} |
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.
Is releases meant to be plural? I might be misremembering but i thought it was singular.
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 I believe their README is incorrect. I submitted a PR to correct it: googleapis/release-please#1057
Based on my output from this test run it's plural: https://github.com/willarmiros/test-package-lock-repo/runs/3617992025?check_suite_focus=true#step:3:4
@dyladan is this ready for merging? |
Which problem is this PR solving?
Short description of the changes
I tested these changes via an identical action in a dummy repo. You can see it completing a successful release here: https://github.com/willarmiros/test-package-lock-repo/runs/3618032476?check_suite_focus=true