-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Auto-update more versions #2351
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.
Oh! I'm so looking forward to reviewing this more closely, but it might not be until later today or tomorrow. If another maintainer approves before then, I'm ok if this lands sooner than later.
I do have some comments about making the script runnable locally, in particular under macOS. I'd made improvements to .github/workflows/scripts/check-collector-version.sh
so that it could be run locally and in a "dry-run" mode. I'd like to preserve that ability. I don't mind submitting followup changes.
Just sharing a few ideas of the top. Will share more later. |
I like that! @chalin I am wondering if we should move those versions into a data/ file and then use from there at the different places, wdyt? (Does not have to be part of this PR) |
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.
Great improvement. See inline comments and questions, thanks.
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
ya, sorry about that, I generally just test jobs in my fork, but I totally support adding those features back |
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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.
Left an inline comment about adding a guard. Otherwise, this LGTM. Thanks!
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.
Approving on account of the question about the guard being addressed
Let me know if you'd like to change anything. After you merge this, it should pick up this lagging
javaVersion
that we haven't been updating as a good test that it's working (I've also tested it in my fork).opentelemetry.io/content/en/docs/instrumentation/java/manual.md
Line 9 in 0f45ace