Document release process and common labels #425
Conversation
source/contribute/maintainer.rst
Outdated
|
|
||
| Internal | ||
| Changes that do not impact the end user, but might impact contributors | ||
| or maintainers, such as improvements to the CI. |
There was a problem hiding this comment.
What other labels should we use across repositories? Should we take this as an occasion to challenge tagging bugfixes with Bug?
Right now we have:
Bug, which could also beBugfix, orPatchNew feature/Enhancement, which could also beMinorBC Break, which could also beMajor
What naming do you prefer for the three things?
Also, we should probably pick colors for each tag we agree on.
There was a problem hiding this comment.
BC Break can also mark a PR of having an unwanted BC break and once it gets fixed, the label can be removed. It can differentiate between BC break and major release.
There was a problem hiding this comment.
But you wouldn't have something tagged as Major if there is no BC break, would you?
There was a problem hiding this comment.
Correct, not the other way around. I see BC Break and Major with two different meanings.
There was a problem hiding this comment.
I agree with @SenseException, BC Break and Major, as well as all other pairs of labels don't have identical meanings. At the same time, it's already clear whether the change is a patch, minor or major from the target branch. Why do we need labels for that?
There was a problem hiding this comment.
On a historical view you can't tell the difference between a patch or a minor by the branch name. In case you want to tell if a one year old PR was either patch or minor, but I don't know if you ever had such a use case.
There was a problem hiding this comment.
Why would I need to? In any event, the major/minor/patch terminology primarily belongs to semver. Using the same terminology for changes, especially if it doesn't exactly match the kind of the version where this change was made will make it hard to comprehend.
There was a problem hiding this comment.
Okay, if this info isn't needed later on, we don't need the release-labels.
greg0ire
force-pushed
the
label-standardization
branch
from
bd619ac
to
2dfe3c3
Compare
source/contribute/maintainer.rst
Outdated
| that are not phpdoc comments. | ||
|
|
||
| Deprecation | ||
| For pull requests that *introduce* a new deprecation. |
There was a problem hiding this comment.
IMO we should find another label for pull requests that address a deprecation.
There was a problem hiding this comment.
Usually addressing a deprecation is a BC Break.
There was a problem hiding this comment.
By "addressing a deprecation", I mean making the deprecation from another package go away, not dropping a legacy code path.
There was a problem hiding this comment.
I see. In this case, isn't it just an internal change? It shouldn't affect the consumers in any way.
There was a problem hiding this comment.
Agree, that would be internal. And that what I want to emphasize it, we shouldn't label such a PR as Deprecation.
There was a problem hiding this comment.
By emphasizing "introduce", you imply that there's something else that this label is not about, but I would have never thought of that being a deprecation in another library. The most natural opposite of "introduce" is "fix" but we've already covered it.
There was a problem hiding this comment.
I added a clarification.
source/contribute/maintainer.rst
Outdated
|
|
||
| Documentation | ||
| Includes changes to rst files, markdown files as well as code comments | ||
| that are not phpdoc comments. |
There was a problem hiding this comment.
Not sure how to label a PR that changes phpdoc comments… "Static Analysis"?
There was a problem hiding this comment.
phpdoc comments become more and more obsolete and are replaced with type casts when it comes to types. But the descriptive content would still be a documentation. I don't know if we should focus more on phpstan/psalm annotations when it comes to types (and therefore "Static Analysis") and regular phpdocs when it's anout documentation. I don't know about possible grey lines though.
There was a problem hiding this comment.
Agree, the description part would still be documentation. Maybe the distinction will be too hard to make.
There was a problem hiding this comment.
IMO, "Static Analysis" has been used for the issues on detecting the issues in the API (e.g. by upgrading a dependency) and fixing the API contracts as a result of static analysis. In my understanding, this label was originally meant to justify the extra burden of running static analysis tools by exposing the issues it allowed to discover.
A change in the API (native parameter/return type) or in the API documentation (phpDoc) that isn't needed to address a static analysis build failure doesn't fall under the "static analysis" category IMO.
I don't know if it is important to distinguish the API and API documentation categories. On the one end, they both are about the API but on the other, a change in the API may be breaking while a change in the documentation cannot.
There was a problem hiding this comment.
A change in (API) documentation can be breaking for our users' CI, can't it? IMO, in that case, there is an impact, and since we are focusing on impact with this label, maybe we should prefer "Bugfix" / "Improvement" for those?
There was a problem hiding this comment.
No, a change in documentation cannot be breaking.
There was a problem hiding this comment.
Can you please elaborate? If we can make the CI of our user turn red, wouldn't you call that breaking?
There was a problem hiding this comment.
Let's assume the following library code:
/**
* @param string $string
*/
private function acceptsString($string);
/**
* @param string|null $value
*/
public function publicFunction($value) {
acceptsString($value);
}And the client code:
/**
* @param string|null $value
*/
function doStuff($value) {
publicFunction($value);
}There is a bug in the publicFunction() documentation that it accepts null while it doesn't. The null case isn't covered by a test, and the CI on the library side isn't strict enough to identify the issue. At the same time, the CI on the client side is strict enough but there's no issue so far.
Now, the library maintainers decide to fix the documentation and change the API documentation to:
/**
* @param string $value
*/
public function publicFunction($value)By doing that, they are not changing the API in any way that would break an existing production scenario. A production scenario that might rely on publicFunction() accepting null couldn't exist. But the client's CI will fail and point to a bug in their API documentation. Nothing more.
The impact of breaking CI by changing API documentation is the same as breaking CI that doesn't allow deprecations by introducing a deprecation (which is also part of API documentation).
There was a problem hiding this comment.
The impact of breaking CI by changing API documentation is the same as breaking CI that doesn't allow deprecations by introducing a deprecation (which is also part of API documentation).
It sure is smaller, but as you wrote yourself, there is an impact. If the issue is accountability, we could still acknowledge that impact without making it part of our backwards compatibility promise. Then I guess it would not be "breaking"? To me, changes in phpdoc fall into 2 categories:
- more precise: "Improvement" (for static analysis)
- more accurate: "Bugfix" (for static analysis)
IMO, "Static Analysis" has been used for the issues on detecting the issues in the API (e.g. by upgrading a dependency) and fixing the API contracts as a result of static analysis. In my understanding, this label was originally meant to justify the extra burden of running static analysis tools by exposing the issues it allowed to discover.
Right now we have 2 Static Analysis tools, so although we primarily act on our phpdoc "as a result of static analysis", doing so for one tool improves the experience when using the other be it in our own CI or in our users' CI, because it becomes able to detect new issues when we are more precise. To me, it might be worth reusing that "Static Analysis" label so that we make another section, which would sit between "Internal" and the rest. Would that make sense?
If we go that way, we should probably not label such PRs with "Improvement" or "Bugfix" so as to avoid having subsections since we cannot generate those yet.
|
I remember we had a topic about unified labels a long time ago somewhere on GitHub, but I didn't find it quickly. |
Sorry, I should have mentioned it. I put it in the description. |
source/contribute/maintainer.rst
Outdated
| that are not phpdoc comments. | ||
|
|
||
| Deprecation | ||
| For pull requests that *introduce* a new deprecation. |
There was a problem hiding this comment.
Usually addressing a deprecation is a BC Break.
source/contribute/maintainer.rst
Outdated
|
|
||
| Internal | ||
| Changes that do not impact the end user, but might impact contributors | ||
| or maintainers, such as improvements to the CI. |
There was a problem hiding this comment.
I agree with @SenseException, BC Break and Major, as well as all other pairs of labels don't have identical meanings. At the same time, it's already clear whether the change is a patch, minor or major from the target branch. Why do we need labels for that?
| assigned to the milestone. | ||
|
|
||
| Labels are used to generate the release notes, and we should strive to | ||
| provide a consistent experience accross repositories. Here are labels |
There was a problem hiding this comment.
IMO, we're jumping right to the solution w/o the due statement of the problem. Why do we even use them in release notes? Why should labels be consistent across repositories? To what extent should they be consistent? What else are labels used for?
It's hard to discuss the solution to the problem that I personally don't understand. As a single-repository maintainer, I mostly use labels for navigation and grouping of related issues. Should we define the scope of this discussion? E.g. which labels should be considered common and which shouldn't?
So far, the only aspect common to all repositories that comes to mind is the upgrade impact (i.e. what the users should expect to be fixed, what's deprecated, and so on). Are there any other common aspects?
There was a problem hiding this comment.
Why do we even use them in release notes?
Good question! Our release notes do not look like the ones on https://keepachangelog.com/en/1.0.0/
I can find old releases that have a format consistent with the one we are currently using, although they were definitely not relying on laminas/automatic-releases. laminas/automatic-release relies on https://github.com/jwage/changelog-generator , so that's probably what was used:
- https://github.com/doctrine/orm/releases/tag/v2.4.8
- https://github.com/doctrine/common/releases/tag/v2.6.0
The other predominant format was one that simply mentioned the number of issues resolved:
I'd say right now we are using labels in release notes to be consistent with the format above, but I don't know the original reason.
Why should labels be consistent across repositories?
I think labels that have the same name should be understood the same way across repositories, so that it's easier to maintain any Doctrine repository. Personally, I've noticed that's sometimes I'm looking for some label in the list, and don't know which to pick. Sometimes "Enhancement" is present, sometimes you have "Improvement"… it's a bit confusing.
Should we define the scope of this discussion? E.g. which labels should be considered common and which shouldn't?
Yes, definitely. Maybe we should only discuss labels that are put on PRs, since they are the ones we are using for generating the changelog
So far, the only aspect common to all repositories that comes to mind is the upgrade impact (i.e. what the users should expect to be fixed, what's deprecated, and so on). Are there any other common aspects?
Nothing comes to mind, but yes the upgrade impact is indeed something we will want to make clear to our users regardless of the repository.
There was a problem hiding this comment.
From the upgrade impact standpoint, the categories defined in the "keepachangelog" make sense to me, and I wouldn't reinvent the wheel.
On the other hand, as far as I can tell, such changelogs are generated manually by committing changes in the changelog to Git. To me, this would be annoying and actually look like an abuse of the VCS which already keeps track of the changes.
Are there examples of maintaining a changelog like this via labels?
There was a problem hiding this comment.
I don't have an example in mind no. People don't tend to label PRs with Added, Removed. The past tense seems weird here, and also a single PR can contain several additions and removals.
On the other hand, as far as I can tell, such changelogs are generated manually by committing changes in the changelog to Git. To me, this would be annoying and actually look like an abuse of the VCS which already keeps track of the changes.
Fully agree, having a CHANGELOG.md file seems weird to me also, having a /releases endpoint does the job quite well. To me, both things are the same, only the medium changes.
From the upgrade impact standpoint, the categories defined in the "keepachangelog" make sense to me, and I wouldn't reinvent the wheel.
I think they do make sense, but that we should map them to labels, because labeling a PR with Added feels weird. I don't think that we can actually map labels to categories, but IMO it's not bad at all to have "New Feature" or "Deprecation" as a category.
- Added => New Feature
- Changed => ??? Internal? BC Break? That one is unclear to me.
- Removed => BC Break
- Deprecated => Deprecation
- Fixes => Bugfix
- Security can stay as is
There was a problem hiding this comment.
This implicit mapping is a slippery slope. I believe the whole point of keeping the changelog is to keep it explicitly and manually. Only a human can decompose an arbitrary set of label combinations into a human-friendly log.
So I believe, we should either use labels w/o trying to adopt the "keepachangelog" principle, or we do it manually (looks like too much work).
There was a problem hiding this comment.
So I believe, we should either use labels w/o trying to adopt the "keepachangelog" principle
If by that you mean using "New Feature", "BC Break" etc. as opposed to "Added", "Changed", etc. Then that sounds good to me, but I'm not sure if that's what you mean because it just seems like different labels but the same principle: letting the user know about the upgrade impact.
There was a problem hiding this comment.
If by that you mean using "New Feature", "BC Break" etc. as opposed to "Added", "Changed", etc. Then that sounds good to me
Yes, let's use just these labels. Maybe we could replace the "New Feature" with a more general "Improvement".
There was a problem hiding this comment.
Just to chime in before writing in detail how we manage this in ODM, I'm personally for "Feature" rather than "Improvement". Multiple things can be improved: performance, DX, API. When we're adding a new feature it's, well, a "Feature" :)
There was a problem hiding this comment.
True. A Feature is an Improvement but an Improvement is not necessarily a feature.
There was a problem hiding this comment.
"Improvement" does feel a bit like it might be an internal feature
greg0ire
force-pushed
the
label-standardization
branch
from
2dfe3c3
to
941f881
Compare
| Changes or removals that may require changes or actions from the user | ||
| before the package can be used again. | ||
|
|
||
| Improvement |
There was a problem hiding this comment.
To be preferred over "New Feature" or "Enhancement" then?
There was a problem hiding this comment.
My vote goes for "Feature". "New" part in new "New feature" is useless, we won't be adding old features ;)
|
Not that I want to interrupt you guys in any way, just a short notice that the four repositories for the Laminas integrations already have their labels standardized. We are using the same labels als laminas does on their repositories, see e.g. https://github.com/doctrine/DoctrineModule/labels. So far, I haven't missed anything there. |
|
Furthermore, I'd like to point to label-syncer, which could be set up in the .github repository to synchronize labels across all Doctrine repositories. The set of labels used in the Laminas-related repositories can be found here. I'd suggest adding such a file to the .github repository, once the discussion about the labels has settled. |
|
That's a nice one 👍 |
|
First things first, list of label we're using in ODM (https://github.com/doctrine/mongodb-odm/labels) filtered to what we're slapping on PRs (sometimes also on issues, not the point):
Comparing with currently discussed list:
Apart from few name differences (and possible quarrels ;) we're good 👍 We also maintain I believe our UPGRADE file policy is also used across other repositories, but since we're trying to standardize labels, those rules also should be included. Also on a closing note: I'm not sure if standardizing labels is a worthwhile thing to do. I'm not saying this to diminish the effort, but because I believe the gain will not be worth the time we invest in discussing this back and forth. Our most important repositories have a group of regular maintainers that can always be consulted or just change labels that were added in a wrong manner. For our important but smaller repositories that are maintained by whoever-has-some-willpower-currently well, inconsistent labeling is least important issue for those repositories ;) |
Is there a point in doing that? I think Bugfix would look better in release notes.
It's not super important indeed, but I think we are close to an agreement, given your answer, and given the restricted scope of this. |
I think so, yes. I tend to link issues against milestones as well, so that they appear in the release notes. Sometimes there is more information available in the issues than there is in a pull request, where the PR is rather on technical aspects (code reviews, please rebase, squash, fix cs issues, etc.) which are not relevant for users. In contrast, issues may hold the discussion on conceptual aspects or how users need to migrate their code. So, yes, I think there are valid reasons for assigning labels to issues and linking issues to milestones. |
|
@driehle yes but is there a point in marking both issues reporting bugs and PRs fixing them as "Bug"? Won't it be confusing to have both under the same section? |
@driehle pointed it out really well :)
I believe our bugfix relases are small enough to not confuse people ;)
Fingers crossed! |
Inspired from https://github.com/orgs/doctrine/teams/doctrinecore/discussions/2