Upgrade website to Docsy 0.5.1 #531
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @SayakMukhopadhyay! |
k8s-ci-robot
added
the
size/XXL
SayakMukhopadhyay
force-pushed
the
upgrade
branch
4 times, most recently
from
bba8ab8 to
dce4971
Compare
|
The biggest "change" in this upgrade is the fact that Bootstrap was updated to v5 by Docsy. This has led to changes to stylistic elements, mostly padding, margins, text sizes and some colours. I will try to document some of them here. (Left is the current site and right is the PR site)
These are all that I have found out. I am not too sure if there is a need to make the upgrade look pixel-accurate to the current site. Also, I have another branch that adds some changes that might need more discussion. These changes include
If the maintainers agree with the above changes, I will include them in this PR. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @SayakMukhopadhyay |
|
Thank you so much!! This will take awhile to review, sorry. |
|
Alright, I will bring in those changes from the other branch to make it easier to review. The config file will contain commented properties which are not needed but I have kept them for now to make it easier to review the YAML->TOML change. I can remove them before merging this PR once the format change itself is reviewed. |
|
This is amazing! Thank you so much. As mentioned, we'll need some time to review all this but, I greatly appreciate the time, effort, and energy. |
|
I have tried to make the commits as atomic as possible so if any changes are required, we can drop those commits. |
|
@SayakMukhopadhyay an aside: if you'd be willing to work on part of the equivalent change for https://k8s.io/, I can make time to pair up with you on that. I'm |
Makefile
Outdated
| dependencies: | ||
| npm ci | ||
| cd themes/docsy/ && npm i |
There was a problem hiding this comment.
Does this need adapting for make container-server?
There was a problem hiding this comment.
Yeah, the Makefile needs to be updated. The dependencies target itself is ok but needs to be added as dependencies on docker-server, container-server and production-build. I have not added them as I wanted to know the sentiment regarding overhauling the Makefile. There is a lot of repetition (like git submodule update --init --recursive --depth 1) that can be moved to its own target and used as a dependency. Let me know if its ok to do that and I will make the changes to this PR.
There was a problem hiding this comment.
Where possible, split out refactorings like that into their own (separate, smaller) PR.
There was a problem hiding this comment.
Which is why I haven't made any changes to the Makefile. The dependencies target though is necessary for the latest Docsy and it was needed to get the preview-build target to work and will be needed for the other targets. Essentially, that command creates 2 folders inside themes, FortAwesome and twbs.
There was a problem hiding this comment.
Does this PR break local container previewing? If so, let's update the README and call out the issue in the PR description.
There was a problem hiding this comment.
By local container previewing, do you mean using make container-server? It doesn't work in master itself. It seems there is a permission issue on the /tmp folder. The following error is thrown:
...
rsync: [receiver] chown "/tmp/src/themes/docsy/userguide/static/images/.version-banner.png.nPONje" failed: Operation not permitted (1)
rsync: [receiver] chown "/tmp/src/themes/docsy/userguide/static/images/.yes.png.iMPnje" failed: Operation not permitted (1)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
make: *** [Makefile:80: container-server] Error 23
This issue came up in WSL and I tried a fresh setup in codespaces and the issue is the same. It seems like the issue dooesn't happen if I remove this line
Line 83 in f42204c
But running the docker commands manually works as expected and so local container previewing still works.
There was a problem hiding this comment.
@sftim From your comment, I dunno how this convo got resolved but yeah, container-server target in the makefile hasn't been updated. In fact, as mentioned in the convo, all the targets needs to be updated to handle the npm dependency. See this message to know my primary concerns.
There are multiple ways this can be approached.
- I can make the changes in the
Makefileincluding adding the dependencies and can keep the refactoring for later. - I can add the dependencies and do the refactoring.
- I can do the refactoring in a separate PR to be merged first and fix the dependencies here so that you can get it working.
For now, for you to test things, I will push a commit adding the dependant target to container-server.
There was a problem hiding this comment.
Also, the rsync permission issue is still there in my system. I have pushed a commit that should get container-server working. If it doesn't try removing lines 87 and 88 (--cap-drop=ALL and --cap-drop=AUDIT_WRITE). Because I can't get things to work without removing them.
I don't know the real issue behind this as I assume it used to work.
There was a problem hiding this comment.
We only support container previewing on Linux and macOS host OSs; if you're using another environment, the support may not have been added yet.
There was a problem hiding this comment.
Hmm...this permission issue occurs on GitHub Codespaces too. But this PR as of now has a new commit that updates the container-server target. Can you check if that works for your local host?
I will split this PR into 2 tomorrow.
hugo.yaml
Outdated
| desc: Discussion and help from your fellow users | ||
| - name: Twitter | ||
| url: 'https://twitter.com/K8sContributors' | ||
| icon: fa-brands fa-x-twitter # Updated the bird icon to the X icon |
There was a problem hiding this comment.
Omit this comment, but once you have, comment on this line in your own PR to call out the change.
There was a problem hiding this comment.
I have removed the comment from the file. Where do you want me to recomment? I have already called out this change in the last line of this PR's description.
Also, what about the other properties that I have commented in hugo.yaml. I have kept those properties commented on to get some feedback, should I remove those properties altogether or should I uncomment them.
Could we add that front matter via the generator? Maybe in a separate PR (which I'd prefer to merge ahead of this one) |
I plan to work on it but I was thinking of doing it in a separate PR as the current workaround works for now. |
package.json
Outdated
| "autoprefixer": "^10.4.20", | ||
| "postcss": "^8.4.47", | ||
| "postcss-cli": "^11.0.0" |
There was a problem hiding this comment.
Aside: do we need any of these outside of dev (eg for a production Netlify build)?
There was a problem hiding this comment.
I don't think so. They have been devDependencies before the PR and stuff worked so I don't think its needed. In fact, autoprefixer and postcss-cli is installed globally in the container (which is probably why it works and is probably not how it should be).
|
@sftim It's fixed. Do you think there is any value in checking in a CI step that the container builds and runs? |
One for a follow-up PR, and maybe best if we involve SIG Testing folks. |
|
Please squash this down to fewer commits; I think that's tidiest. |
|
Alright, the couple of warnings have been addressed. I will squash this PR to 3 commits, 1 containing all the upgrades, 1 containing the submodule -> npm change and the last containing the misc changes. |
There was a problem hiding this comment.
A local preview doesn't work for me. Try removing node_modules before you test @SayakMukhopadhyay
| postcss-cli | ||
| COPY package*.json ./ | ||
|
|
||
| RUN npm ci |
There was a problem hiding this comment.
I think this will create /src/node_modules, but when the repository is mounted over that, the node modules will be shadowed / hidden.
There was a problem hiding this comment.
Welp....that's right. Maybe I should move the npm ci into hack/gen-content.sh.
Seems like k8s/website too has this issue.
There was a problem hiding this comment.
In fact, in the website, npm ci is in the Dockerfile (see https://github.com/kubernetes/website/blob/30350f69b1b1f258a4534be4e16f966a641b4f35/Dockerfile#L44). The way this works is that in the Makefile, each directory is mounted explicitly (see https://github.com/kubernetes/website/blob/main/Makefile#L16) and the whole directory isn't mounted as a whole.
So, we have 2 ways to work this.
- We can use the website way of doing things.
- I can remove the
npm cifrom theDockerfileand put it in theMakefileas
container-server: ## Run Hugo locally within a container, available at http://localhost:1313/
# no build lock to allow for read-only mounts
$(CONTAINER_RUN) -p 1313:1313 \
--mount type=tmpfs,destination=/tmp,tmpfs-mode=01777 \
--read-only \
--cap-drop=ALL \
--cap-drop=AUDIT_WRITE \
$(CONTAINER_IMAGE) \
bash -c 'cd /src && hack/gen-content.sh --in-container && \
cd /tmp/src && \
npm ci && \
hugo server \
--environment preview \
--logLevel info \
--noBuildLock \
--bind 0.0.0.0 \
--buildDrafts \
--buildFuture \
--disableFastRender \
--ignoreCache \
--destination /tmp/hugo \
--cleanDestinationDir'
(note the npm ci && \).
There was a problem hiding this comment.
We can use the website way of doing things.
Let's do that. Otherwise, npm is still running in the host context, and the trust base for our [static site generation website preview] code becomes much larger.
|
This doesn't yet look right @SayakMukhopadhyay. Here's the error I get when I try a local preview: make container-server
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
# no build lock to allow for read-only mounts
docker run --user : --rm -it -p 1313:1313 \
--read-only --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/.git,target=/src/.git,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/assets,target=/src/assets,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/content,target=/src/content,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/external-sources,target=/src/external-sources,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/hack,target=/src/hack,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/layouts,target=/src/layouts,readonly --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/static,target=/src/static,readonly --mount type=tmpfs,destination=/tmp,tmpfs-mode=01777 --mount type=bind,source=/home/tim/Documents/Repositories/kubernetes/contributor-site/hugo.yaml,target=/src/hugo.yaml,readonly \
--cap-drop=ALL \
--cap-drop=AUDIT_WRITE \
k8s-contrib-site-hugo \
bash -c 'cd /src && hack/gen-content.sh --in-container && \
cd /tmp/src && \
hugo server \
--environment preview \
--logLevel info \
--noBuildLock \
--bind 0.0.0.0 \
--buildDrafts \
--buildFuture \
--disableFastRender \
--ignoreCache \
--destination /tmp/hugo \
--cleanDestinationDir'
Copying source repository
rsync: [generator] chown "/tmp/src/." failed: Operation not permitted (1)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
make: *** [Makefile:92: container-server] Error 23 |
|
@sftim as discussed in Slack, the issue is due to the I have also tried running podman myself without the I have added a commit which removes P.S. we should add a CI job to build and test run the container. |
k8s-ci-robot
added
the
needs-rebase
SayakMukhopadhyay
force-pushed
the
upgrade
branch
from
8daa6b9 to
9de4626
Compare
k8s-ci-robot
removed
the
needs-rebase
|
PR rebased |
|
Would you be willing to squash this to a few commits @SayakMukhopadhyay? That's tidier than the many commits here, and it will help people looking at how to do future upgrades. |
SayakMukhopadhyay
force-pushed
the
upgrade
branch
from
9de4626 to
f72085f
Compare
|
@sftim squashed to 2 commits, 1 with the docsy upgrade and the other with the npm module upgrade. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SayakMukhopadhyay, sftim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Great work everyone! |
|
+1 and kudos to @SayakMukhopadhyay and @sftim, this was epic!! |
|
Successor PR: #563 |
This PR updates Docsy to the latest version available and makes it compatible to use with the latest Hugo.This PR was initially made to be upgraded to Docsy 0.10.0 but has since been modified to upgrade only upto 0.5.1. Further upgrade will be done in another PR. I have crossed out the items that was planned in this PR but won'
t happen in this PR either due to being fixed by another PR or being siloed to another PR.
In details, the following are the changes:
v0.10.0 tagv0.5.1 tagUpdated dependencies in thepackage.jsonto the latestautoprefixerandpostcss-cliand addedpostcsswhich is needed bypostcss-clinow.Update the Hugo version in netlify to 0.134.3 andaddedNODE_VERSION = 20.16.0env var to force netlify to use the current LTS of Node.js as older versions won't be able to compile native code in some of the dependencies. (If this value is configured on the infra side, we should look into either updating the value there and remove it from here, or remove it from infra and keep it here.)(WIP: right now, only added as a dependency of thepreview-buildtarget to get the CI in the PR to work)head.html: it was the same as the theme except without the_internal/google_news.htmltemplate. This template has since been removed.navbar.html: it was the same as the theme except removing the minification of the navbar logo due to Hugo not havingtdewolff/minify >= 2.7.3. The latest Hugo hasv2.20.37so that removal is not needed.sidebar-html: it was the same as the theme except wrapping anorconditional method withcondmethod to work with newer versions of Hugo which Docsy was not supporting. The latest Docsy version does support the latest version of Hugo and this wrapping is no longer needed.docs&layout: this layout was an exact copy from the theme with the only difference being thepage-meta-lastmod.htmlpartial was removed in the override. But the partial itself doesn't activate unless both.GitInfoandSite.Params.github_repoare truthy, which is not, hence the override is not needed.events: this layout was unnecessary as all the content files in theeventsfolder are usingtype: docsin its front matter and so theeventslayout will never be used. And anyway,docslayout andeventslayout were exactly the same.calendar: updated to the currentdocslayout provided by the theme while keeping the customization of including some scripts.community: updated to the currentdocslayout provided by the theme as it was the same before. (The only reason this layout is needed is because all generated files incontent/en/communitydoesn't contain thetypeorlayoutin its front matter and thus will default totype: pageif acommunitylayout is not created.)resources: updated to the currentdocslayout provided by the theme as it was the same before. (The only reason this layout is needed is because of a generated filecontent/en/resource/release/index.mdwhich doesn't contain thetypeorlayoutin its front matter and thus will default totype: pageif aresourceslayout is not created.)_variables.scsshas been updated from the theme. This change is introduced by docsy updating to bootstrap 5 in 0.7.0. This change introduces multiple changes to the default layout, including margins, padding, colors, font sizes among others._variables.scsshas been replace by_variables_project.scsswhich is the recommended way to customize the variables of Docsy. This file only lists the properties that need to be customized and not everything in Docsy's_variables.scss_styles_project.scsshas been added to include the CSS to make the copyright statement visible. This is the file that all customisations should go to.In the cover page, the color parameter is removed else it is setting the wrong text color.The Hugo config has been changed from TOML to YAML(This has been done in feat: move from a toml hugo config to a yaml one #556). ~~Some properties which are redundant have also been commented out. Also, since FontAwesome was upgraded to v6 in Docsy, the icon names have been updated. Also fixes Update X logo to 𝕏 on the website's footer #503 ~~Makefileto not use anygit submodulecommands and update theDockerfileto usenpm cito install the dependencies instead of installing them globally.