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

doc,tools: get altDocs versions from CHANGELOG.md #27661

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Parse CHANGELOG.md for versions of Node.js used by the documentation
feature View another version so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 12, 2019
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself LGTM while I would prefer not to use a fixture. That could be done by
a) not reading the whole changelog entry but only a small top part of the file
b) by checking that the current output is the "minimum" expectation instead of the absolute truth and that there could be more versions and more lts entries.

That way it would be easier to detect changes that might break this (e.g. parts of the file structure would be changed or old entries would be moved). That is not as likely but it still seems a possibility.

But I'm also fine with the code as is. This is definitely an improvement over the current situation!

@richardlau
Copy link
Member Author

richardlau commented May 12, 2019

I've just realised this isn't going to work as expected outside of master because we don't port the CHANGELOG.md changes backwards (e.g. https://github.com/nodejs/node/blob/v8.x/CHANGELOG.md doesn't know of any version greater than 8). We do port the altDocs changes, e.g. for Node.js 8:

node/tools/doc/html.js

Lines 170 to 181 in 07458ba

const versions = [
{ num: '11.x' },
{ num: '10.x', lts: true },
{ num: '9.x' },
{ num: '8.x', lts: true },
{ num: '7.x' },
{ num: '6.x', lts: true },
{ num: '5.x' },
{ num: '4.x', lts: true },
{ num: '0.12.x' },
{ num: '0.10.x' }
];

which allows docs for, e.g. Node.js 8, to point to newer versions, e.g. Node.js 10.

@richardlau
Copy link
Member Author

I've just realised this isn't going to work as expected outside of master because we don't port the CHANGELOG.md changes backwards (e.g. https://github.com/nodejs/node/blob/v8.x/CHANGELOG.md doesn't know of any version greater than 8). We do port the altDocs changes, e.g. for Node.js 8:

node/tools/doc/html.js

Lines 170 to 181 in 07458ba

const versions = [
{ num: '11.x' },
{ num: '10.x', lts: true },
{ num: '9.x' },
{ num: '8.x', lts: true },
{ num: '7.x' },
{ num: '6.x', lts: true },
{ num: '5.x' },
{ num: '4.x', lts: true },
{ num: '0.12.x' },
{ num: '0.10.x' }
];

which allows docs for, e.g. Node.js 8, to point to newer versions, e.g. Node.js 10.

And I've just realised that the table for Node.js 8 doesn't match the table in master:

node/tools/doc/html.js

Lines 396 to 407 in b230833

const versions = [
{ num: '11.x' },
{ num: '10.x', lts: true },
{ num: '9.x' },
{ num: '8.x', lts: true },
{ num: '7.x' },
{ num: '6.x', lts: true },
{ num: '5.x' },
{ num: '4.x' },
{ num: '0.12.x' },
{ num: '0.10.x' },
];
because we haven't backported #20904 (which removed the LTS label from Node.js 4).

@BridgeAR
Copy link
Member

@richardlau we could check the file in the repository instead. That's not a great solution but it would at least always work in case there's a network connection. That way it's always up to date.

@richardlau
Copy link
Member Author

@richardlau we could check the file in the repository instead. That's not a great solution but it would at least always work in case there's a network connection. That way it's always up to date.

Yeah, less than ideal but probably okay? I'll do that later (after I've eaten).

I've pushed a change to remove the test fixture and do sanity checks on the versions (e.g. LTS releases are never odd numbered releases) rather than a deepStrictEqual assertion.

@richardlau richardlau added the wip Issues and PRs that are still a work in progress. label May 12, 2019
@richardlau
Copy link
Member Author

@BridgeAR Quick thought -- If we're going to need a network connection we might be better off reading schedule.json from the Release repository (with some checks that the version start date isn't in the future to avoid picking up unreleased versions, e.g. v13) rather than relying on the changelog?

@BridgeAR
Copy link
Member

@richardlau I thought about that as well and it is indeed the perfect data. The reason why I still prefer the changelog approach right now is that it's another repository and we update that file manually. We did lack behind before (just a bit but still) and might forget to update that file at times while that would not happen with the changelogs.

@richardlau richardlau removed the wip Issues and PRs that are still a work in progress. label May 13, 2019
@nodejs-github-bot

This comment has been minimized.

@richardlau
Copy link
Member Author

@richardlau I thought about that as well and it is indeed the perfect data. The reason why I still prefer the changelog approach right now is that it's another repository and we update that file manually. We did lack behind before (just a bit but still) and might forget to update that file at times while that would not happen with the changelogs.

We did forget to update the list of versions in the changelog, see #27414 (this step is in the release process docs being proposed in #25497). I've kept with parsing the changelog for now (albeit the code now fetches the version from the master branch) since I'd already written the parsing regular expressions.

Since the code now fetches from the master branch building the docs and the newly added test will make an Internet request.

cc @nodejs/documentation

@BridgeAR
Copy link
Member

@richardlau we could also just parse the filenames instead of reading the entries from the main changelog.md. I doubt that we'll ever forget a to add such entry. But I'm actually happy with either implementation as long as it automates the process.

The test itself should probably be moved to test/internet.

@richardlau
Copy link
Member Author

@richardlau we could also just parse the filenames instead of reading the entries from the main changelog.md. I doubt that we'll ever forget a to add such entry. But I'm actually happy with either implementation as long as it automates the process.

The test itself should probably be moved to test/internet.

@BridgeAR We won't be able to determine LTS status by just parsing filenames.

I've moved the test to test/internet and also added a test for the previous semver major (i.e. if master is v13.0.0-pre it checks for 12.x). This additional test should catch the cases where we forget to update the main changelog (i.e. when 13.x is released and master moves to v14.0.0-pre the test will fail if Node.js 13 is not added to the main changelog).

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@ChALkeR
Copy link
Member

ChALkeR commented May 14, 2019

#27658 (comment) is also relevant here -- the LTS labels in current master don't seem correct.

4.x and 6.x are both EOL (and both were LTS previously), so they should have identical labelling (either both have LTS label or both not have it, or both have an EOL label).

Refs: #20904 nodejs/Release#440

@richardlau
Copy link
Member Author

#27658 (comment) is also relevant here -- the LTS labels in current master don't seem correct.

4.x and 6.x are both EOL (and both were LTS previously), so they should have identical labelling (either both have LTS label or both not have it, or both have an EOL label).

Refs: #20904 nodejs/Release#440

I pushed an update to #27658 that makes the LTS status consistent (both 4 and 6 no longer have it). Once the changelog is consistent this PR will make the LTS labels in the "View another version" drop down in the built docs consistent automatically (provided we keep the changelog accurate).

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

@richardlau This is ready, right? Do you want to go ahead and land this?

@richardlau
Copy link
Member Author

@richardlau This is ready, right? Do you want to go ahead and land this?

I think it was. It’s a long weekend here in UK (public holiday on Monday) so it’ll be Tuesday at the earliest before I can land this.

Parse `CHANGELOG.md` for versions of Node.js used by the documentation
feature `View another version` so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.
@richardlau
Copy link
Member Author

@richardlau This is ready, right? Do you want to go ahead and land this?

I think it was. It’s a long weekend here in UK (public holiday on Monday) so it’ll be Tuesday at the earliest before I can land this.

node-core-utils says this isn't ready to land because changes were pushed after the review. Taken the opportunity to rebase and squash the fixup commits and address @Trott 's suggested changes to avoid the use of sanity (as in sanity checks) in the comments.

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added review wanted PRs that need reviews. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 29, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented May 30, 2019

Still LGTM.

@richardlau
Copy link
Member Author

Still LGTM.

I'd assumed (perhaps naively) that this LGTM should have satisfied node-core-utils, but it does not:

-bash-4.2$ git-node land 27661
✔  Done loading data for nodejs/node/pull/27661
----------------------------------- PR info ------------------------------------
Title      doc,tools: get altDocs versions from CHANGELOG.md (#27661)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     richardlau:autoaltdocs -> nodejs:master
Labels     doc, review wanted, tools
Commits    1
 - doc,tools: get altDocs versions from CHANGELOG.md
Committers 1
 - Richard Lau <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/27661
Reviewed-By: Rich Trott <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc,tools: get altDocs versions from CHANGELOG.md
   ℹ  Last Full PR CI on 2019-05-29T12:26:38Z: https://ci.nodejs.org/job/node-test-pull-request/23453/
   ℹ  This PR was created on Sun, 12 May 2019 17:14:09 GMT
   ✔  Approvals: 1
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/27661#pullrequestreview-236953915
--------------------------------------------------------------------------------
? This PR is not ready to land, do you want to continue?

There's a couple of reasons for this:

  1. The comment in question doesn't get recognized by the regular expression https://github.com/nodejs/node-core-utils/blob/1e9daeb15475efdfea4a418686395299ad892149/lib/reviews.js#L7. I'd be a little wary relaxing the regexp as, for example, I'm not sure we'd want to treat the LGTM in doc,tools: get altDocs versions from CHANGELOG.md #27661 (review) as an approval.
  2. As far as I can tell the code that checks commits since the last review only looks at reviews and not LGTM's (assuming they were detected) in the comments.

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 1, 2019
Parse `CHANGELOG.md` for versions of Node.js used by the documentation
feature `View another version` so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

PR-URL: nodejs#27661
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Jun 1, 2019

Landed in 4ec6135

@Trott Trott closed this Jun 1, 2019
targos pushed a commit that referenced this pull request Jun 1, 2019
Parse `CHANGELOG.md` for versions of Node.js used by the documentation
feature `View another version` so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

PR-URL: #27661
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Jun 3, 2019
@richardlau richardlau deleted the autoaltdocs branch November 2, 2019 08:49
richardlau added a commit to richardlau/node-1 that referenced this pull request Apr 6, 2020
Parse `CHANGELOG.md` for versions of Node.js used by the documentation
feature `View another version` so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

Backport-PR-URL: nodejs#32642
PR-URL: nodejs#27661
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 6, 2020
Parse `CHANGELOG.md` for versions of Node.js used by the documentation
feature `View another version` so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

Backport-PR-URL: #32642
PR-URL: #27661
Reviewed-By: Rich Trott <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 7, 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. review wanted PRs that need reviews. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants