-
Notifications
You must be signed in to change notification settings - Fork 411
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
feat: auto update ipfs version in installation docs #1081
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
7b92a84
to
fe98368
Compare
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.
nit: If you place your substitution part after update http-api-docs
section, you'd be free to cd $ROOT/docs
.
Needs small fixes but overall I think it'd be a quick win here if the maintainers are OK with it. Another approach would be to adopt some proper templating engine for docs generation and use ipfs tag as an input but that's a major undertaking compared to this change so I think we can put it off for now.
# update installation docs | ||
while read -r file; do | ||
echo "replacing $CURRENT_IPFS_TAG with $LATEST_IPFS_TAG in $file" | ||
sed -E -i "s/$CURRENT_IPFS_TAG/$LATEST_IPFS_TAG/g" $file |
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.
Is a situation in which we'd want references to $CURRENT_IPFS_TAG
survive a new IPFS tag release? Some that I can think of:
- referring to an older version of IPFS to highlight some differences between the versions
- referring to a version of some other thing that just happens to be the same as
$CURRENT_IPFS_TAG
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.
👍 Definitely limitations of not using a real templating system.
My assumption is that these are unlikely to happen or might happen once, which will catch at review time.
Because we have the frontmatter requirement and we only transform current version
to new version
.
(if we document something about versionX specifically, it will be replaced only when we upgrade ipfs from versionX to versionY).
(will let you and the maintainers ack on this)
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.
Cool with me, thank you for the reply :) That's 👍 from me, I'll let you follow up with the maintainers now.
@@ -1,6 +1,7 @@ | |||
--- | |||
title: Command-line | |||
description: Using IPFS through the command-line allows you to do everything that IPFS Desktop can do, but at a more granular level since you can specify which commands to run. Learn how to install it here. | |||
current-ipfs-version: v0.12.0 | |||
--- |
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.
Commenting here so we get a thread & trace some research:
Needs small fixes but overall I think it'd be a quick win here if the maintainers are OK with it. Another approach would be to adopt some proper templating engine for docs generation and use ipfs tag as an input but that's a major undertaking compared to this change so I think we can put it off for now.
Agreed, thanks for the feedback @galargh,
FWIW:
There is also a templating feature in vuepress,
but the syntax seems to break markdown URLs. This won't be transformed into a link:
[my link](https://mysite.com/{{ $frontmatter.myVariable }})
And we can't use templating in code blocks.
We could write vuepress plugins, but it would be more time-consuming & invasive.
fe98368
to
a2abd67
Compare
a2abd67
to
43aa51f
Compare
|
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 looks good from my end. Might wanna get a SWE to double check stuff though.
@lidel do you mind giving this PR a look over? My automation skills are pretty lax. |
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.
LGTM, but it seems CI shell is no longer bash
and we also had dependabot enabled with impacted this workflow.
See follow-up fixes in #1121
while read -r file; do | ||
echo "replacing $CURRENT_IPFS_NUMBER with $LATEST_IPFS_NUMBER in $file" | ||
sed -E -i "s/$CURRENT_IPFS_NUMBER/$LATEST_IPFS_NUMBER/g" $file | ||
done <<< "$(grep "current-ipfs-version" ./docs -R --files-with-matches)" |
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.
@laurentsenta turns out when <<<
is used we need to make sure we use bash
and not sh
– see fix in #1121
current-ipfs-version
frontmatter option to make the feature opt-in,