Skip to content
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

are the YAML "added" annotations ambiguous? and are they rewritten on backport? #24850

Closed
sam-github opened this issue Dec 5, 2018 · 15 comments
Labels
doc Issues and PRs related to the documentations.

Comments

@sam-github
Copy link
Contributor

  • Version: all node.js docs
  • Platform:
  • Subsystem:

cf. #24847

The added annotations are very useful, and at the same time, I think they are slightly ambiguous, or at least their interpretation is subtle! And undocumented: https://nodejs.org/api/documentation.html at least doesn't describe them, but perhaps they are described elsewhere?

Examples:

Added: v7.5.0

Is the API present in 8.0.0? I think it would have to be, because if it was added in >8.0.0 and backported then this would look like:

Added:
- v8.3.0
- v7.5.0

Correct? But a bit subtle!

Its not clear to me whether @nodejs/releasers have a process to update the YAML tags when backporting/cherry-picking. Because #24847 I suspect not. It would be kind of onerous, since both the backported branch and the master needs to be updated to reflect the release of the API on the old branch. I don't know how much energy should be spent in keeping the annotations up to date on the release branches, but it would be great if at least the master & current documentation was complete.... meaning it would need updating after every release. :-(

Summary: I think that ideally:

  1. the "about this documentation" docs should describe the meaning of the YAML version annotations.
  2. we should have tooling/process to keep those annotations up to date.
@sam-github sam-github added the doc Issues and PRs related to the documentations. label Dec 5, 2018
@sam-github
Copy link
Contributor Author

@nodejs/docker

@SimenB

This comment has been minimized.

@chorrell

This comment has been minimized.

@targos
Copy link
Member

targos commented Dec 5, 2018

The problem is not exactly with the release process. When the PR lands on master, it should already include a REPLACEME placeholder in the docs metadata. Then when releasers include the change, that placeholder is replaced with the actual version number. The release commit that does it is cherry-picked to master.

@sam-github
Copy link
Contributor Author

Sorry at-nodejs/docker! I meant @nodejs/documentation .

@targos
Copy link
Member

targos commented Dec 5, 2018

The main issue is that we forget too often to include the placeholder in semver minor changes

@addaleax
Copy link
Member

addaleax commented Dec 5, 2018

Yeah, I would assume that what releasers have done so far when cherry-picking the release commit to master from an older (e.g. LTS) release line, is to solve the merge conflict with the existing added: marker from a newer release line by overriding it, instead of merging the added: tags?

@sam-github
Copy link
Contributor Author

@targos that happens when a change is released in a "current" release branch, but what about when it is released further, into the LTS branches?

@targos
Copy link
Member

targos commented Dec 5, 2018

@sam-github I have never prepared an LTS release so I'm not sure but this part of the process should be identical.
As @addaleax said, maybe sometimes releasers removed a conflicting line instead of merging them.

@sam-github
Copy link
Contributor Author

@addaleax the process you describe seems like it might work... any reason why it didn't for URL? #24847 predates that process? The process is not documented? The process wasn't followed?

@targos that part of the process can't be identical, can it? When picking from master to current, there are REPLACEME tags in the markdown files. When picking from current to LTS, there will never be REPLACEME tags in the markdown files, I don't think, since those tags were removed in the initial release. Do I misunderstand?

@targos
Copy link
Member

targos commented Dec 5, 2018

Anyway, one thing that we currently don't do, is cherry-picking between release lines. So in the current state, if no mistake was done and a change happened in v11.4.0 and later in v10.15.0, only the documentation on master would have both "added" fields. Release docs would only include their corresponding version

@targos
Copy link
Member

targos commented Dec 5, 2018

When picking from current to LTS

Is it what LTS releasers do? In that case you're right

@sam-github
Copy link
Contributor Author

Actually, maybe it isn't, because the REPLACEME does seem to get updated on LTS branches.

I don't have the time to do a full audit, but the first SEMVER-MINOR in CHANGELOG_V10.md is servers.headerTimeout, here's the situation with it, probably its typical:

  • v11&master docs: added in v11.3.0, but you can't tell if it has been backported to any release lines
  • v10 docs: added in v10.14.0, so you can't tell if it was first added in v10.14.0 and will be present in all v11.x, or if it was added in v10.14.0 and backported to v8/v6, or added in v11 and backported to v10

The docs look to me like they contain info only for their release lines, unless someone manually, like @watson in #24847, manually updated them, which is pretty onerous. Also, there aren't any docs on the meaning of the tags, so the current situation is not obvious. Sometimes the docs do have info about old release lines, so anybody reading the docs could justifiably assume that when the info is missing, its because the feature wasn't backported.

@sam-github
Copy link
Contributor Author

@nodejs/lts ----^

@targos
Copy link
Member

targos commented Jun 14, 2020

I think this was fixed in #33042

@targos targos closed this as completed Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

5 participants