-
Notifications
You must be signed in to change notification settings - Fork 906
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
[Proposal] A pre-release review process for new major releases of any client library #1418
Conversation
025c951
to
c64376b
Compare
Can I ask why does TC think it needs such strict gating process? |
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 start on the TC helping regularize the release process.
Some suggestions -
- Add an overall purpose statement for this pre-release review process - what does this process aim to accomplish, outline benefits to the project.
- Add a checklist or list of questions maintainers can use for a spec compliance review.
- Add a table to outline process milestones and timelines.
A few reasons from the top of my head:
There is probably more. |
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 be great fort consistency! I'd like to add some kind of commitment to do these reviews swiftly, but could leave that out.
Committee members are not language experts, and it is expected to work with the | ||
language maintainers and/or invited language experts to perform this review | ||
process. Language maintainers SHOULD respond to any question during the review | ||
time in a reasonable amount of time to not delay the review process. |
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.
Later in the doc we mentioned "every decision will be made in less than 14 days after the process is started", so there is no way a language maintainer could delay the process.
Perhaps we can rephrase this to "All the questions and feedbacks will be communicated as GitHub issues filed against the language client repo. If the language maintainers don't respond before the deadline (which is two weeks after the initial filing of the PR), the PR will be closed as REJECTED.".
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 14days the deadline for the ENTIRE process (beginning of process, TC review, language contributors addressing/fixing change, new release), or just the deadline for the contributors' response?
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.
@lzchen that is our hope, but we will probably learn more during the first few reviews.
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.
@bogdandrutu
I asked for clarification between two options. Could you clarify which one it is? xD
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.
TC aims and will put effort into finishing the entire process in 14 days, for this language maintainers need to be responsive :) I think this is how I would rephrase everything
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.
@lzchen that is our hope, but we will probably learn more during the first few reviews.
If the goal is to learn by trying out the process with few reviews, we should go through OTEP first.
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.
OTEP of what? I don't think oteps are necessary good for processes, I think if this was a feature I would completely agree with you. Otep is a process itself, we must go deeper :)
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 suggest that we pick two language clients that are willing to try out this process, go through the steps proposed in this PR until they release a stable version. After two language clients released, we can take the learning/feedback and see whether we should move forward or back out.
Whether this PR should be here or somewhere else (e.g. in the community repo) - I don't know, probably not here as it is weird to put some process document and request form in a spec repo (unless the process is about the spec itself). No strong opinion though.
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.
that is what we will do naturally, not all the languages are ready for this. So not sure if anything is needed to be said 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.
Most of my comments are non-blocking.
One blocker I'm seeing - in this PR we've introduced new terms without definition, and some of them seem to be different from what we've already defined - e.g. telemetry type
vs. signal
.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c64376b
to
6148831
Compare
From @tigrannajaryan : A few reasons from the top of my head: To help language SIGs to find out whether they are ready for GA.
There is a TODOs section in the template
Not sure I can think of how this would look like, can you provide a small example |
For all maintiners, please do not release a stable version until a decision is made on this PR: @open-telemetry/cpp-maintainers @open-telemetry/go-maintainers @open-telemetry/dotnet-maintainers @open-telemetry/erlang-maintainers @open-telemetry/java-maintainers @open-telemetry/javascript-maintainers @open-telemetry/php-maintainers @open-telemetry/python-maintainers @open-telemetry/ruby-maintainers @open-telemetry/rust-maintainers @open-telemetry/swift-mainteiners |
820d7f9
to
bf3da24
Compare
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 am in favor of the formalized process with a checklist to follow / template doc to fill out. But I'm not as certain that the TC must always be involved. We're operating on the honor system. A language team could just release a new version without following the process. If they do follow the process then I would rather trust them to follow it all the way, without an extra friction of involving TC.
@yurishkuro point taken. But I think it is hard to come with that list without going through the process couple of times to identify all the requirements. Alternatively I can see us just requiring the checklist and as a best effort TC members do what I suggest here once or twice and bring the learnings to the process back |
That's fair, we can certainly do a few test runs, especially since many TC members are actually involved in some of the SIGs. I would just prefer to take it easy on the MUST wording for TC involvement (I am ok with MUST for the process itself). |
LGTM - as there are still standing questions (and doubts), I suggest we give this a shot to a few volunteering SIGs and document our findings/feedback in further PRs to tune this process. |
Co-authored-by: Tom Tan <lilotom@gmail.com>
Co-authored-by: Tom Tan <lilotom@gmail.com>
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
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.
To make it clear: for me this process is a way for TC members to help language SIGs to make a release, to help ensure the language implementation is consistent with other languages, confirms with the spec. We want the TC member to be another pair of fresh eyes who can help the SIG to go over the release checklist.
There is absolutely no intent to introduce a process that "controls" the language SIGs or introduces obstacles in the release process. If any of the language SIGs notice that the process is not helpful the TC would like to know about it immediately so that we can fix (or eliminate) the process.
@tigrannajaryan Personally, I find the wording of the PR to be quite aggressive and way more formal than it needs to be. Having to go through a process just adds more overhead onto the maintainers and TC member(s) responsible for reviewing. I am totally fine with working offline directly with a TC member as a maintainer to discuss any of the logistics, but a whole process seems to be overkill. The only benefit I see from having a process is that it will encourage reviewers/approvers/maintainers/TC members to prioritize the tasks outlined in the "review" process as something to be done as soon as possible. The two week timeline "lights a fire" under contributors and I think that is valuable (it certainly caused Python contributors to prioritize a lot of the tasks that @carlosalberto had found). Just my thoughts from the first few days of going through this process "informally". |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
After trying this out with the Python SIG, I suggest with this along the following points:
@bogdandrutu Sounds like a good compromise? And in general, having this document as a general starting point is something really valuable to have, even if not all SIGs end up doing it. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
The main idea is that before every major release or the release of a new telemetry type (metrics)
we would like to do a technical and specification conformance review that will ensure consistency
and same "look and feel" for all officially released libraries.