-
Notifications
You must be signed in to change notification settings - Fork 81
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
Updates Hugo & removes vendored docsy #284
Conversation
498f4d6
to
360a70b
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.
A few notes on changes.
@@ -192,14 +192,14 @@ flowchart TB | |||
|
|||
A(v0.0.1) --> B(v0.0.4) | |||
{{</mermaid>}} | | |||
| skips | `ID(<bundle tag>) x--x | <versions that should be skipped> | ID(<bundle tag>)` | {{<mermaid>}} | |||
| skips | `ID(<bundle tag>) x--x \| <versions that should be skipped> \| ID(<bundle tag>)` | {{<mermaid>}} |
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.
Newer versions of Hugo use goldmark to render markdown with an extension which implements GitHub Flavored Markdown: Tables spec.
Accordingly to the spec pipe in a cell’s content needs be escaping (example).
As a result of proper spec implementation in goldmark tables & graphs get broken with new versions of Hugo.
- name: Install Hugo CLI | ||
run: | | ||
wget -O ${{ runner.temp }}/hugo.deb https://github.com/gohugoio/hugo/releases/download/v${HUGO_VERSION}/hugo_extended_${HUGO_VERSION}_linux-amd64.deb \ | ||
&& sudo dpkg -i ${{ runner.temp }}/hugo.deb | ||
- name: Checkout | ||
uses: actions/checkout@v3 | ||
- name: Install Node.js dependencies | ||
run: npm ci | ||
- name: Build with Hugo | ||
run: hugo |
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.
Previously we were running Hugo in a container (see hack/ci/link-check.sh
) using klakegg/hugo
image, but new versions of Hugo are missing. So now we are installing hugo from github releases.
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 have no particular problem with the idea of shifting the mechanism if we need to, but I see versions up to 0.101.0 available in docker, so the intention was to access versions closer to HEAD (0.113.0) here?
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.
@grokspawn this PR updates Hugo to 0.111.1. It was the latest version at the moment of creating of this PR. I'm happy to update it to the latest version once we merge this PR. It should be significantly smaller PR.
As far as I see Hugo doesn't maintain official docker image and klakegg/hugo
is maintained by an individual. Despite most likely there is nothing wrong with it - i'm hesitant to use unofficial image.
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.
It is pretty popular on GitHub though: https://github.com/klakegg/docker-hugo
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.
@anik120 I mean... This package is also popular - https://www.npmjs.com/package/is-odd. It got 431k downloads each last week. What it basically does is num % 2 == 1
. What are you implying?
When there is a choice between a poorly maintained random docker image from the internet vs 2 lines of bash which get an official release artefact... Choice is obvious to me.
${CONTAINER_ENGINE} volume create ${volume_name} | ||
${CONTAINER_ENGINE} run --rm ${CONTAINER_RUN_EXTRA_OPTIONS} -v "$(pwd):/src" -v ${volume_name}:/src/public klakegg/hugo:0.73.0-ext-ubuntu |
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.
See comment in .github/workflows/lint.yaml
for more details on this.
@@ -24,7 +24,7 @@ | |||
{{ $p := .page }} | |||
{{ $shouldDelayActive := .delayActive }} | |||
{{ $active := eq $p.CurrentSection $s }} | |||
{{ $show := or (and (not $p.Site.Params.ui.sidebar_menu_compact) ($p.IsAncestor $s)) ($p.IsDescendant $s) }} | |||
{{ $show := or (and (not $p.Site.Params.ui.sidebar_menu_compact) ($p.IsAncestor $s)) (or (eq $p $s) ($p.IsDescendant $s)) }} |
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.
There were breaking changes in Hugo in relatiton to what IsAncestor
returns which required an extra condition.
@@ -1,4 +1,5 @@ | |||
{{ .Page.Store.Set "hasmermaid" 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.
Makes Docsy to include mermaid scripts & styles on pages which use this shortcut. Apperently in older versions it was enabled globaly.
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.
@m1kola this is great!!!! 🎉 only thing that worries me is that it's a HUMONGOUS that is doing 2/3 things. If you can find some time do so, it's going to be much easier to review these changes if this PR is broken up into 3/4 PRs that each does one particular thing
I manually walked the netlify presentation versus the existing docs, and the only question I have is: do we /have/ to have the comment at the bottom of each page that identifies the PR merge which last modified that page? |
360a70b
to
2325ee8
Compare
@anik120 I'm a big fan of small PRs and I tried to break it down (see #282 and #283 for example). However I beleive we can not break it down any further into smaller PRs which are mergable into master: too many things depend on each other. E.g.:
We can probably create a new base branch for these changes and split this PR into smaller PRs which won't be buildable until we merge all of them. While working on this I treid to create commits which contain smaller pieces of work. Could you please try to review commit by commit instead of full PR diff? I'm sorry that I didn't suggest it earlier. If reviewing commit by commit is still not manageble - please let me know and we will discuss options.
@grokspawn hmm, good catch! However I'm looking at the live version and I see that on some pages (1, 2, for example) have "Last modified" and some don't (e.g. this one). I think we should be constent and either show it everywhere or hide it everywhere. What do you think? Rebased on top of master. Please take another look. |
My preference would be to not show it at all, but I don't think either outcome should block this. |
6d30d08
to
f6860bd
Compare
Rebased on top of master to fix conflicts with #289 |
f6860bd
to
e09e3fd
Compare
# ignore | ||
# 1: links going back to help.github.com are rate-limited and can make this flaky | ||
# 2: docsy autogenerated edit links to original markdown source, which will fail if the markdown file is new | ||
${CONTAINER_ENGINE} run --rm -v ${volume_name}:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/' | ||
${CONTAINER_ENGINE} run --rm -v $(pwd)/public:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/,/tree\/master\/.*\.md/,/new\/master\/.*filename=change-me\.md/' |
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 implies that we now have to have a local install of hugo to run, right? (e.g. brew install hugo
) How are we capturing that new requirement & workflow? For e.g., I would always be able to run this script locally to build+validate changes before review, but first I'll have to install hugo now and manually run something?
I guess this goes back to my earlier comment about how close to hugo HEAD we need to be, because it's just 0.101.0 < 0.113.0 that isn't exposed in the available containers.
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.
@grokspawn that was actually my initial goal (not of this PR, but overall): I wanted to be able to run this locally. master
branch has readme which is hugoly outdated:
- It talks about a git submodule for docsy, but there is no submodule. Theme commited into the repo. And there is some mention of
https://github.com/google/docsy-example.git
in readme... Like - how is it supposed to help me run it locally? How is it related? - It assumes that you already have hugo installed.
- It says that you must have version 0.45 (extended). However there is no
darwin-arm64
binary of Hugo for versions which work witholm-docs
. I tried to compile it myself and gave up. Docker image also fails ondarwin-arm64
.
Even ignoring darwin-arm64
related issues - we do not have this captured today. But once we merge this - I'm happy to look into improving local experience with olm.
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.
Created #301 to track these other updates needed, so that this PR doesn't get any more complex. 😄
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.
Okay. Sounds like we can avoid a massive PR like this if we just update the README then?
It talks about a git submodule for docsy, but there is no submodule. Theme commited into the repo. And there is some mention of https://github.com/google/docsy-example.git in readme... Like - how is it supposed to help me run it locally? How is it related?
This is the submodule: https://github.com/operator-framework/olm-docs/tree/master/themes/docsy, which you do remove in a commit further up the chain too.
Removing the theme from being committed in the repo: what benefit do we get other than saving a little space in the git sever? I.e looking for justification for investing building/review cycles for this effort.
The mention of https://github.com/google/docsy-example.git is clearly a typo, if you read the statement above, it's trying to show the example of cloning the olm-docs repo, but with --git-submodules
on.
It assumes that you already have hugo installed.
Looks like there's a missingGetting started
page that should be pointing to https://gohugo.io/installation/macos/ for Mac installations.
It says that you must have version 0.45 (extended). However there is no darwin-arm64 binary of Hugo for versions which work with olm-docs. I tried to compile it myself and gave up. Docker image also fails on darwin-arm64.
It says you must have version 0.45 at least. I am going to try installing Hugo on my machine from that link above and go through the steps to see how things go.
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.
Okay. Sounds like we can avoid a massive PR like this if we just update the README then?
No. This whole thing started because I couldn't run the docs site locally. I was forced to create a draft PR and wait for CI to build and publish a preview for me. After spending some time trying to make old version of Hugo work on darwin-arm64
I just gave up and decided to update the thing.
This is the submodule: https://github.com/operator-framework/olm-docs/tree/master/themes/docsy, which you do remove in a commit further up the chain too.
No, this is not a submodule. Maybe it used to be a submodule, but at some point someone committed it and made changes to the vendored code.
Removing the theme from being committed in the repo: what benefit do we get other than saving a little space in the git sever?
I have this in the PR description:
Vendored docsy theme was modified with project-specific changes (scss, layout changes) which made it really hard to update. This PR switches the project to use go modules for vendoring Hugo dependencies. See docsy docs for more details.
I'm not concerned about saving space on GitHub (especially given that this removal doesn't remove anything from the history and technically doesn't save any space). What I'm concerned about is when people accidentally modifying vendored code. It is like committing vendored go modules into your repo and then making changes in ./vendor
directory. This either prevents you from updating your dependencies (because you modified them) or makes you lose your changes.
I.e looking for justification for investing building/review cycles for this effort.
Justification is - I couldn't run the thing locally. Not a great dev experience. On top of that - we are using a legacy service (docsearch v2). This is not to mention a bunch of other related tech debt reasons.
After this change we can fix some pain points and open the door for further improvements. For example we can share a theme between sdk.operatorframework.io and olm.operatorframework.io: the only difference is a few colours. This way we can maintain a consistent look in feel between the two.
It says you must have version 0.45 at least. I am going to try installing Hugo on my machine from that link above and go through the steps to see how things go.
There is no binary for darwin-arm64
and if you try to compile Hugo yourself - it fails on compiling dependencies.
Instead of spending time on this I suggest that we proceed with the changs and leave 5 years old version of Hugo behind.
Switches to go modules Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
The "variables" in the project were overloading an scss file with "variables" from the docsy theme which was breaking scss transpilation Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
Signed-off-by: Mikalai Radchuk <[email protected]>
e09e3fd
to
6db50ab
Compare
@anik120 did you have any specific concerns, or was the 'change requested' just about the PR size? |
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 there could be value in updating Hugo to something > v0.100.0 from the current v0.45.0, although that does still sound like a low priority effort, since the website is running without any glitches, and there hasn't been any news about deprecation/removal of the v0.45.0 image (which I assume would not be possible to remove anyway).
<style> | ||
.DocSearch-Container { | ||
z-index: calc(var(--of--header-main) + 1); | ||
} | ||
</style> |
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.
Why change 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.
Stuff like these definitely don't have to be in the same PR though. "Change the look and feel of the search box" is not mentioned anywhere in the PR description/commit messages and yet these changes are there included in the commits.
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.
@anik120 PR description mentions this:
Switches the project to docsearch v3 (v2 is no longer maintained + Docsy assumes v3 usage now)
https://docsearch.algolia.com/docs/legacy/dropdown/
This specific style is required so that search popup of docsearch v3 doesn't get covered by header (it wasn't an issue in v2 due to different placement of v2 search overlay).
This is why smaller PRs are always favored. It's really tough to verify everything that's going in in PRs of this size.
Because this repo is so outdated it is impossible (or at least I do not see a way) to split it into smaller changes. You change one thing - you have to change another. It is a can of worms.
|
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.
There are a LOT of these sprinkled all over, which makes it really hard to focus on the actual changes. Lot of brain power is just needed for filtering out the noise. I'm guessing this is a text editor setting that needs to be fixed
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.
@anik120 I agree - people who commit a bunch of spaces at the end of lines should fix their editors :)
In the meantime - git diff upstream/master -w
(or --ignore-space-at-eol
) should help with that.
Github UI also has a setting for this. I hope this helps.
- name: Install Hugo CLI | ||
run: | | ||
wget -O ${{ runner.temp }}/hugo.deb https://github.com/gohugoio/hugo/releases/download/v${HUGO_VERSION}/hugo_extended_${HUGO_VERSION}_linux-amd64.deb \ | ||
&& sudo dpkg -i ${{ runner.temp }}/hugo.deb | ||
- name: Checkout | ||
uses: actions/checkout@v3 | ||
- name: Install Node.js dependencies | ||
run: npm ci | ||
- name: Build with Hugo | ||
run: hugo |
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.
It is pretty popular on GitHub though: https://github.com/klakegg/docker-hugo
# ignore | ||
# 1: links going back to help.github.com are rate-limited and can make this flaky | ||
# 2: docsy autogenerated edit links to original markdown source, which will fail if the markdown file is new | ||
${CONTAINER_ENGINE} run --rm -v ${volume_name}:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/' | ||
${CONTAINER_ENGINE} run --rm -v $(pwd)/public:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/,/tree\/master\/.*\.md/,/new\/master\/.*filename=change-me\.md/' |
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.
Okay. Sounds like we can avoid a massive PR like this if we just update the README then?
It talks about a git submodule for docsy, but there is no submodule. Theme commited into the repo. And there is some mention of https://github.com/google/docsy-example.git in readme... Like - how is it supposed to help me run it locally? How is it related?
This is the submodule: https://github.com/operator-framework/olm-docs/tree/master/themes/docsy, which you do remove in a commit further up the chain too.
Removing the theme from being committed in the repo: what benefit do we get other than saving a little space in the git sever? I.e looking for justification for investing building/review cycles for this effort.
The mention of https://github.com/google/docsy-example.git is clearly a typo, if you read the statement above, it's trying to show the example of cloning the olm-docs repo, but with --git-submodules
on.
It assumes that you already have hugo installed.
Looks like there's a missingGetting started
page that should be pointing to https://gohugo.io/installation/macos/ for Mac installations.
It says that you must have version 0.45 (extended). However there is no darwin-arm64 binary of Hugo for versions which work with olm-docs. I tried to compile it myself and gave up. Docker image also fails on darwin-arm64.
It says you must have version 0.45 at least. I am going to try installing Hugo on my machine from that link above and go through the steps to see how things go.
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 there could be value in updating Hugo to something > v0.100.0 from the current v0.45.0
@anik120 If we update Hugo from v0.45.0 to v0.100.0 - we will still have to make all the changes in this PR... Well this gives us a chance to continue using an unofficial Hugo image instead of 2 lines of bash, but I would like to avoid using random images in our supply chain if possible.
although that does still sound like a low priority effort, since the website is running without any glitches
Yes, it is mostly static site and will likely be fine for some time (apart from legacy v2 search which can be taken down at any time). However, working with it locally is not possible today for ARM Mac users. By not upgrading we just accumulate more technical debt and make future upgrade harder. It is just delaying inevitable.
and there hasn't been any news about deprecation/removal of the v0.45.0 image (which I assume would not be possible to remove anyway)
IMO this repo is in terrible shape: 5 years old Hugo release, changes to the vendored code, no way to easily preview docs locally on ARM Macs and search relies on a legacy Algolia version.
The reason why this PR is so large is because it's an overdue renovation. I opened a can of worms. If we can keep this repo up to date in the future - we can avoid huge diffs. But now there is very little we can do to avoid large changes.
|
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.
@anik120 I agree - people who commit a bunch of spaces at the end of lines should fix their editors :)
In the meantime - git diff upstream/master -w
(or --ignore-space-at-eol
) should help with that.
Github UI also has a setting for this. I hope this helps.
<style> | ||
.DocSearch-Container { | ||
z-index: calc(var(--of--header-main) + 1); | ||
} | ||
</style> |
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.
@anik120 PR description mentions this:
Switches the project to docsearch v3 (v2 is no longer maintained + Docsy assumes v3 usage now)
https://docsearch.algolia.com/docs/legacy/dropdown/
This specific style is required so that search popup of docsearch v3 doesn't get covered by header (it wasn't an issue in v2 due to different placement of v2 search overlay).
This is why smaller PRs are always favored. It's really tough to verify everything that's going in in PRs of this size.
Because this repo is so outdated it is impossible (or at least I do not see a way) to split it into smaller changes. You change one thing - you have to change another. It is a can of worms.
- name: Install Hugo CLI | ||
run: | | ||
wget -O ${{ runner.temp }}/hugo.deb https://github.com/gohugoio/hugo/releases/download/v${HUGO_VERSION}/hugo_extended_${HUGO_VERSION}_linux-amd64.deb \ | ||
&& sudo dpkg -i ${{ runner.temp }}/hugo.deb | ||
- name: Checkout | ||
uses: actions/checkout@v3 | ||
- name: Install Node.js dependencies | ||
run: npm ci | ||
- name: Build with Hugo | ||
run: hugo |
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.
@anik120 I mean... This package is also popular - https://www.npmjs.com/package/is-odd. It got 431k downloads each last week. What it basically does is num % 2 == 1
. What are you implying?
When there is a choice between a poorly maintained random docker image from the internet vs 2 lines of bash which get an official release artefact... Choice is obvious to me.
# ignore | ||
# 1: links going back to help.github.com are rate-limited and can make this flaky | ||
# 2: docsy autogenerated edit links to original markdown source, which will fail if the markdown file is new | ||
${CONTAINER_ENGINE} run --rm -v ${volume_name}:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/' | ||
${CONTAINER_ENGINE} run --rm -v $(pwd)/public:/target mtlynch/htmlproofer /target --ignore-empty-alt --ignore-status-codes 429 --allow-hash-href --ignore-urls '/help.github.com/,/edit\/master\/.*\.md/,/tree\/master\/.*\.md/,/new\/master\/.*filename=change-me\.md/' |
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.
Okay. Sounds like we can avoid a massive PR like this if we just update the README then?
No. This whole thing started because I couldn't run the docs site locally. I was forced to create a draft PR and wait for CI to build and publish a preview for me. After spending some time trying to make old version of Hugo work on darwin-arm64
I just gave up and decided to update the thing.
This is the submodule: https://github.com/operator-framework/olm-docs/tree/master/themes/docsy, which you do remove in a commit further up the chain too.
No, this is not a submodule. Maybe it used to be a submodule, but at some point someone committed it and made changes to the vendored code.
Removing the theme from being committed in the repo: what benefit do we get other than saving a little space in the git sever?
I have this in the PR description:
Vendored docsy theme was modified with project-specific changes (scss, layout changes) which made it really hard to update. This PR switches the project to use go modules for vendoring Hugo dependencies. See docsy docs for more details.
I'm not concerned about saving space on GitHub (especially given that this removal doesn't remove anything from the history and technically doesn't save any space). What I'm concerned about is when people accidentally modifying vendored code. It is like committing vendored go modules into your repo and then making changes in ./vendor
directory. This either prevents you from updating your dependencies (because you modified them) or makes you lose your changes.
I.e looking for justification for investing building/review cycles for this effort.
Justification is - I couldn't run the thing locally. Not a great dev experience. On top of that - we are using a legacy service (docsearch v2). This is not to mention a bunch of other related tech debt reasons.
After this change we can fix some pain points and open the door for further improvements. For example we can share a theme between sdk.operatorframework.io and olm.operatorframework.io: the only difference is a few colours. This way we can maintain a consistent look in feel between the two.
It says you must have version 0.45 at least. I am going to try installing Hugo on my machine from that link above and go through the steps to see how things go.
There is no binary for darwin-arm64
and if you try to compile Hugo yourself - it fails on compiling dependencies.
Instead of spending time on this I suggest that we proceed with the changs and leave 5 years old version of Hugo behind.
/lgtm |
Prerequisites:
.RelPermalink
from.URL
#283This PR:
Vendored docsy theme was modified with project-specific changes (scss, layout changes) which made it really hard to update. This PR switches the project to use go modules for vendoring Hugo dependencies. See docsy docs for more details.
Next steps (follow up PRs):