-
Notifications
You must be signed in to change notification settings - Fork 495
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
Stop using deprecated update-munge-docs.sh for TOC generation. #247
Conversation
FWIW this worked flawlessly for me and @jessfraz for the 1.5.2 and 1.4.8 releases LGTM |
@ixdy any comments here? I'd like to get this one in soon. |
# Generate a (github) markdown TOC between BEGIN/END tags | ||
# @param file The file to update in place | ||
# | ||
common::mdtoc () { |
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 a little sad that we've converted tested Go code to untested shell. :\
anchor=${anchor//[\.\?\*\,\/()]/} | ||
# Keep track of dups and identify | ||
((count[$anchor]++)) ||true | ||
((${count[$anchor]}>1)) && anchor+="-${count[$anchor]}" |
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.
this line is wrong.
Looking at https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md#client-binaries, the Client Binaries anchor is https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md#client-binaries, followed by https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md#client-binaries-1, etc.
per the logic on https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/toc_filter.rb#L43-L44, probably need to increment count after we use it.
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.
PTAL.
# Keep track of dups and identify | ||
if [[ -n ${count[$anchor]} ]]; then | ||
((count[$anchor]++)) ||true | ||
((${count[$anchor]}>0)) && anchor+="-${count[$anchor]}" |
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 conditional might not be necessary - $count[$anchor]
should always be greater than zero, since it was incremented on the previous line, right?
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.
Ah yes, that was a vestige of the earlier logic. Thanks. Removed.
anchor=${anchor//[\.\?\*\,\/()]/} | ||
# Keep track of dups and identify | ||
if [[ -n ${count[$anchor]} ]]; then | ||
((count[$anchor]++)) ||true |
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.
probably don't need the ||true
, though it does no harm.
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.
Still needed. Thank you "set -e"!
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 guessing someone should update the CHANGELOG with the fix, since the header links are incorrect right now.
@ixdy if the links are functional and it's just a matter of labeling, we can leave this until the next release which will update the TOC. |
@david-mcmahon the links are wrong - they take you to the wrong section. |
well, some are. e.g. on https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md, if you click "Client Binaries" for 1.4.8, you instead end up under 1.5.1. |
Automatic merge from submit-queue (batch tested with PRs 39832, 40660) Complete *-munge-docs.sh deprecation. **What this PR does / why we need it**:\ Complete *-munge-docs.sh deprecation. TOC generation now handled by kubernetes/release#247 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # ref #38309 **Special notes for your reviewer**: cc @bgrant0607 @thockin
…raft Add latest release notes to draft document
cc @saad-ali @jessfraz