-
Notifications
You must be signed in to change notification settings - Fork 678
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
README includes 1.0 release checklist + CODEOWNER description #596
README includes 1.0 release checklist + CODEOWNER description #596
Conversation
Did you read this OTEP https://github.com/open-telemetry/oteps/blob/main/text/0155-external-modules.md? |
@lonewolf3739 I just gave it a quick read through! Thanks for pointing it out, I didn't notice it before. I didn't find anything that would cause there to be a need for updates to this PR though. There's already a link to the membership guidelines, and we mention that packages need tests. Let me know if there's something you wanted to add? |
Wondering how did we come up with time to respond in 2-3 business days and what makes someone eligible for the owner of the package? Transfer of ownership? Since many of the problems related to management are common across SIGs, I believe it is not a good idea for us to define what it means to be X unless that is specific to SIG. I think some spec-level guidelines with flexibility and autonomy to SIG maintainers would be the ideal way to go forward. |
@lonewolf3739
I took inspiration from a similar proposal on a OTel JS PR, although that PR was shared responsibility of "vendor specific packages", I thought the same could apply to "instrumentation expert packages"
We discussed this in the SIG, and this PR attempts to put those ideas into action. For example @lzchen had asked me to add "what it means to be a CODEOWNER" and I added my interpretation of that in the description of the
From what I've seen from other repos, especially for contrib items, this is specific to each SIG. I think what we discussed during the SIG is a safe and good option, (i.e. what we add in this checklist) and wouldn't see it necessary to block this checklist because specifications hasn't defined the expected checklist for releasing contrib packages as 1.0
I think I would disagree with this specific for the contrib repo. The way I saw contrib repos is as a place for users to find useful packages for OTel without the same level of guarantees and robustness in the core repos. The specs don't usually robustly define contrib actions because it's supposed to let people move independent of specification progress. i.e. in our case where we want release "contrib" packages as 1.0. For example, .NET contrib has 5 maintainers compared to the 2 we have on Python. Not saying it's too many or too little, just that the "flexibility" you mentioned it achieved with many different experts without the need of specs? |
We can't expect the same kind of turnaround time for people who volunteer to help maintain a package.
https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/creating-a-repository-on-github/about-code-owners. The people you choose as code owners must have write permissions for the repository. It says they should have maintainer access and I don't think the community guidelines for maintainership should apply here. |
Sure that's fair. Do you think we should remove this constraint? Again this is just a draft to get the conversation started. I purposely left out "How to be removed as a CODEOWNER" because I think our future experience should decide what we care about.
I believe that is not true, CODEOWNERS need write access, not maintainer access, which is more powerful according to this GitHub matrix. However I haven't looked into the specifics. I plan to look into how other OTel SIGs are solving this, but I suspect at a minimum they will need to give write access. When it comes to releases, we can make it through a GitHub workflow or something which would only require "Write access". There are other solutions for releases but at the very least I'm not suggesting we give CODEOWNERS maintainer access... yet :) |
6b71dba
to
5c8cf42
Compare
README.md
Outdated
|
||
Maintainers aim to periodically release new versions of the packages in `opentelemetry-python-contrib`. | ||
|
||
Contributions that enhance OTel for Python are welcome to be hosted upstream for the benefit of group collaboration. However, as a general rule, releases for packages in `opentelemetry-python-contrib` will be in a perpetual `~=0.X.0` (pre-1.0) version state. This is because maintainers and approvers cannot feasibly provide the stability guarantees that are implied with a 1.0 release for so many packages. |
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 find this and the next paragraph a bit hard to understand, the way they are written kind of make me believe that generally, the contrib packages will be perpetually in 0.X.0
. This can be a bit misleading because we may perfectly end up having a codeowner for every package, thus having every package in a A.B.C
version where A > 0
.
An explanation on why we (maintainers and approvers) can't be codeowners for every contrib package (and thus many may remain as 0.X.Y
) is fine, but I think a short comment and the list of requirements below is enough for our purposes.
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 way they are written kind of make me believe that generally, the contrib packages will be perpetually in
0.X.0
Yes that is the intention as we discussed in the last SIG meeting. Contrib packages will stay in 0.X.0
UNLESS someone becomes a CODEOWNER and wants to take it to 1.0
.
That's why we include the following paragraph right after:
To resolve this, members of the community are encouraged to commit to becoming a CODEOWNER for packages in `-contrib` that they feel experienced enough to maintain. CODEOWNERS can then follow the checklist below to release `-contrib` packages as 1.0 stable:
This can be a bit misleading because we may perfectly end up having a codeowner for every package, thus having every package in a A.B.C version where A > 0.
I'm not sure if we discussed this. Even if we find CODEOWNER's for every package, the previous paragraph should still be taken (as it is written) "as a general rule" because when new packages get added, they won't have CODEOWNERs and so it will be in 0.X.0
.
but I think a short comment and the list of requirements below is enough for our purposes.
Let me know if you have suggestions on how the paragraph can be more clear! 🙂 I tried to say it as concisely as possible but definitely open to feedback on how else it can be reworded!
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 now even thinking that having a codeowner is a necessary but not sufficient condition for a package to step from 0.X.Y to 1.X.Y. Also, packages may get to a version greater than 0.X.Y and then they may lose their codeowner. I think both these points should be included in the explanation in this document.
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 agree that having a CODEOWNER is not a sufficient condition to release a package as 1.0. I think we are tracking that in #591 under "Remaning Issues". This PR just resolves #590 which is one of the issues. Once they are all resolved, we'll have a final path forward for releasing contrib packages as 1.0
Also, packages may get to a version greater than 0.X.Y and then they may lose their codeowner. I think both these points should be included in the explanation in this document.
Are you referring to when CODEOWNERs leave the project? I didn't add anything about that in this file because we don't really have precedent regarding what we should do... It shouldn't be a major problem because once a package is release as 1.0 we won't have to "un-release" it. They can leave the project and the work can just stay at "1.X" or something and we won't update until we find a new CODEOWNER.
I don't think we need to answer the question of them leaving just yet, but let me know what points you would like to see if you think otherwise?
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'm not sure about document this as stated either. I don't think releasing 1.0 should depend on the package having a codeowner. We could totally review a package and release 1.0 for it if we feel confident about the public API.
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.
We can release stable version meaning we guarantee we won't make breaking API changes to the package but it doesn't promise support for future bug fixes/enhancements IMO.
What you said makes sense, we agreed in the SIG meetings that a CODEOWNER is not required, but is encouraged so this PR is updated to reflect that.
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.
Thanks. IIRC, we had also discussed about 1.0 representing stability guarantees in the sense that we won't break the packages but it doesn't mean the package has a maintainer or is officially supported meaning there is no promise to fix bugs/issues. Do you think we should remove the "perpetual 0.x" statement?
As a user, I'd be much happier to use a package if it works now and guarantees it won't break until the next major release even if it means my reported bugs may not be fixed in a timely manner. On the other hand, I'd be very reluctant to use a 0.X.
release as it indicates constant churn.
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.
On the other hand, I'd be very reluctant to use a 0.X. release as it indicates constant churn.
Sorry do you mean to literally take a dependency as opentelemetry-instrumentation-flask~=0.0
? (Implying I don't care if it is 0.24.0
or 0.25.0
for example). If that's what you meant then yeah I agree, I would be reluctant to take that kind of dependency. I was thinking they would take a more strict dependency like ~=0.24.0
if they know it works for them. Then they can ignore the future 0.X.0
updates if they want.
I can remove the "perpetual 0.x" statement, but I think that's more so describing what we already do rather than suggesting a new pattern. Do you mean that in the near future we will stop releasing all the packages together, moving them to the next 0.X
even though they have no changes? That line was more so just to explain what we are doing right now, but if we won't do it anymore then we can definitely remove that comment.
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.
My only feedback is that we should not say that packages will generally never hit 1.0 as that is not true. We may or may not release a 1.0 for any package in contrib depending on whether we feel confident about not making breaking changes in future. In another comment you mentioned this:
With that said, maintainers will look for things like good documentation, good unit tests, and in general their own confidence when deciding to release a package as 1.0.
Which to me sounds something very different from the following
However, as a general rule, releases for packages in
opentelemetry-python-contrib
will be in a perpetual~=0.X.0
(pre-1.0) version state.
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 understand what you're saying, I've updated the PR to remove the "perpetual" 1.0
statement and used the quote you mentioned.
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.
People come and go in OSS. Do we have a plan for when a codeowner is no longer able to keep up with the project? Or guidelines for how to pass on or orphan a package?
I am having more thoughts about requiring a codeowner for a package to be >= 1.0.0. I now think having a codeowner and the version number are separate things, for the reason you mention above. I have no expectation that a specific codeowner may remain or leave or transfer ownership or not. We should just keep the codeowner file up to date to reflect the fact that there is a codeowner or not and pretty much that's it. |
I thought without precedent it wouldn't make sense to add that just yet. We can always add it later as I mentioned in my reply to Diego's comment here.
I agree with you, I mentioned in my reply to you here that we have #591 to track what it means to release a version number. A CODEOWNER is just part of it.
I agree with that. This PR is just a small piece of the puzzle by writing a checklist for what a CODEOWNER is 🙂 Releasing a package will be solved when #591 is solved. Since this is just a small part of the larger issue, please let me know what other comments you have so that we can get this merged? |
d050177
to
2613723
Compare
8fe1c6e
to
088f074
Compare
I agree with the goals here but not so sure about coupling versioning/stability with codeownership. I think what we need to optimize for is the following:
An example would be Elastic wanting to maintain ElasticSearch instrumentation. This would allow them to ensure the instrumentation follow Elastic best practices and is always up to date. At the same time Otel maintainers/approvers can still help ensure it confirms to the Otel project. Whether the package should release 1.0 or not should be decided by project and package maintainers both on a case to case basis. |
I would say the goal of this PR is to say that versions
This PR says that all that would be needed from the individual is for them to
We're trying to do that in #598 as a first step, but yeah I agree this is an important goal and hopefully that PR can help us towards it.
Agreed, this PR just adds some conditions that we discussed (i.e. the maintainers said they wanted) before a package gets released as 1.0. |
@NathanielRN |
.github/CODEOWNERS
Outdated
# - update usage of `opentelemetry-python-core` APIs upon the introduction of breaking changes | ||
# | ||
# A CODEOWNER SHOULD: | ||
# - be a member of the OpenTelemetry community |
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.
Isn't this a requirement as codeowners will have to be approvers and be able to push tags?
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'm not sure if we said that a CODEOWNER must be an approver. CODEOWNERS won't be able to push tags either because main
is a protected branch, so that is only done by maintainers.
Take suggestion package "MUST" wording. Co-authored-by: Diego Hurtado <[email protected]>
Take suggestion on CODEOWNER requirement wording. Co-authored-by: Diego Hurtado <[email protected]>
Take more suggestions on checklist list requirements. Co-authored-by: Diego Hurtado <[email protected]>
Take suggestion on MUST requirement for documentation.
@lzchen Sure! We said that CODEOWNERS are encouraged, but they are not required for releasing contrib packages as With that said, maintainers will look for things like good documentation, good unit tests, and in general their own confidence when deciding to release a package as The changes in this PR have been updated to reflect these points. Please review! @ocelotl @owais |
# | ||
# A CODEOWNER SHOULD: | ||
# - be a member of the OpenTelemetry community | ||
# - be a member of the OpenTelemetry community so that the `component-owners.yml` action to automatically assign CODEOWNERS to PRs works correctly. |
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.
Our component-owners.yml
action says this in its README:
This action only requires that component owners be collaborators on the repository or members of the organization, but does NOT require they have write access.
So we highlight that membership is required.
This reverts commit a0fe538.
Description
In preparation for releasing instrumentation packages as 1.0, we add a path forward for members of the community who want to help release instrumentation packages as 1.0.
This PR adds a description of what it means to be a CODEOWNER, deletes a superfluous CONDEWONERS file, and updates the README to tell the community what we decided is the checklist for moving a package to 1.0.
Fixes #590
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
N/A
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
- [ ] Changelogs have been updated- [ ] Unit tests have been added