contributor-site icon indicating copy to clipboard operation
contributor-site copied to clipboard

Upgrade website to Docsy 0.10.0 and Hugo 0.134

Open SayakMukhopadhyay opened this issue 1 year ago • 11 comments

This PR updates Docsy to the latest version available and makes it compatible to use with the latest Hugo.

In details, the following are the changes:

  • Updated the docsy submodule to use v0.10.0 tag
  • Updated dependencies in the package.json to the latest autoprefixer and postcss-cli and added postcss which is needed by postcss-cli now.
  • Update the Hugo version in netlify to 0.134.3 and added NODE_VERSION = 20.16.0 env 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.)
  • Update the Makefile to add a target to download the dependencies of docsy. (WIP: right now, only added as a dependency of the preview-build target to get the CI in the PR to work)
  • Partials removed:
    • head.html: it was the same as the theme except without the _internal/google_news.html template. 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 having tdewolff/minify >= 2.7.3. The latest Hugo has v2.20.37 so that removal is not needed.
    • sidebar-html: it was the same as the theme except wrapping an or conditional method with cond method 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.
  • Layouts removed:
    • docs & layout: this layout was an exact copy from the theme with the only difference being the page-meta-lastmod.html partial was removed in the override. But the partial itself doesn't activate unless both .GitInfo and Site.Params.github_repo are truthy, which is not, hence the override is not needed.
    • events: this layout was unnecessary as all the content files in the events folder are using type: docs in its front matter and so the events layout will never be used. And anyway, docs layout and events layout were exactly the same.
  • Layouts updated:
    • calendar: updated to the current docs layout provided by the theme while keeping the customization of including some scripts.
    • community: updated to the current docs layout provided by the theme as it was the same before. (The only reason this layout is needed is because all generated files in content/en/community doesn't contain the type or layout in its front matter and thus will default to type: page if a community layout is not created.)
    • resources: updated to the current docs layout provided by the theme as it was the same before. (The only reason this layout is needed is because of a generated file content/en/resource/release/index.md which doesn't contain the type or layout in its front matter and thus will default to type: page if a resources layout is not created.)
  • ~~_variables.scss has 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.scss has been replace by _variables_project.scss which 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.scss has 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. 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 #503

SayakMukhopadhyay avatar Sep 21 '24 15:09 SayakMukhopadhyay

Welcome @SayakMukhopadhyay!

It looks like this is your first PR to kubernetes/contributor-site 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/contributor-site has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot avatar Sep 21 '24 15:09 k8s-ci-robot

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)

  1. The page title is no longer capitalised by default (see https://github.com/google/docsy/pull/797) but the title and menu items are now aligned. image
  2. In the welcome page, margins are calculated based on screensize instead of being harcoded in bootstrap. Should these margins be changed to reflect the current state? Do note, the current site is using defaults. Also, the link colours are a bit different. image
  3. The blog list also has very minor margin differences. Also, note the slight difference in the link colours. Another difference that is present in all list pages is that the left hand side navigation looks a little different with an underline below the page title instead of the items being indented and the search box being squarish. image
  4. Disabled buttons in pagination also have a different colour image
  5. Feedback buttons look a bit different image

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

  1. ~~Moving to YAML config for Hugo instead of TOML as Docsy itself has moved and well...YAML is the language of K8s right 😄~~ (This has been done in #556) ? Also, while moving, I figured out which config variables are redundant due to them just repeating the default values. Removing those properties can help make it more maintainable. I have commented those right now but depending on the consensus, I can remove them.
  2. Deletion of _variables.scss as it just redefines values in the Docsy theme with maybe a couple of changes. Instead, I have added _variables_project.scss which is the recommended way to customise variables by Docsy. This file only has what is needed to be overridden.

If the maintainers agree with the above changes, I will include them in this PR.

SayakMukhopadhyay avatar Sep 22 '24 14:09 SayakMukhopadhyay

/assign @SayakMukhopadhyay

SayakMukhopadhyay avatar Sep 22 '24 15:09 SayakMukhopadhyay

Thank you so much!! This will take awhile to review, sorry. Personally: +1 on moving from toml to yaml and also on deletion of _variables.scss.

tokt avatar Sep 23 '24 03:09 tokt

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.

SayakMukhopadhyay avatar Sep 23 '24 05:09 SayakMukhopadhyay

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.

chris-short avatar Sep 27 '24 20:09 chris-short

I have tried to make the commits as atomic as possible so if any changes are required, we can drop those commits.

SayakMukhopadhyay avatar Sep 28 '24 08:09 SayakMukhopadhyay

@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 @sftim on Kubernetes' Slack workspace.

sftim avatar Sep 28 '24 09:09 sftim

community: updated to the current docs layout provided by the theme as it was the same before. (The only reason this layout is needed is because all generated files in content/en/community doesn't contain the type or layout in its front matter and thus will default to type: page if a community layout is not created.)

Could we add that front matter via the generator? Maybe in a separate PR (which I'd prefer to merge ahead of this one)

sftim avatar Sep 28 '24 09:09 sftim

community: updated to the current docs layout provided by the theme as it was the same before. (The only reason this layout is needed is because all generated files in content/en/community doesn't contain the type or layout in its front matter and thus will default to type: page if a community layout is not created.)

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.

SayakMukhopadhyay avatar Sep 28 '24 10:09 SayakMukhopadhyay

Also see https://github.com/kubernetes/website/pull/48726

sftim avatar Dec 22 '24 16:12 sftim

I pulled this locally and found that make container-server doesn't succeed. Per https://github.com/kubernetes/contributor-site/pull/531/files#r1827530755, I don't recommend merging this as-is.

Here's the error I saw:

Error: command error: failed to load modules: module "github.com/FortAwesome/Font-Awesome" not found in "/tmp/src/themes/github.com/FortAwesome/Font-Awesome"; either add it as a Hugo Module or store it in "/tmp/src/themes".: module does not exist
make: *** [Makefile:84: container-server] Error 1

My guess is that we need to ensure that the Docsy dependencies are built into the container image (this is what we're planning in https://github.com/kubernetes/website/pull/48812)

Would you like more advice on how to fix things here @SayakMukhopadhyay? You might find it easiest to bump to 0.5 first and then send in a smaller PR with the bump from 0.5 to 0.10


PS. Once this is good to merge, would you be willing to squash this down to 1 or 2 commits @SayakMukhopadhyay ?

sftim avatar Dec 22 '24 16:12 sftim

I pulled this locally and found that make container-server doesn't succeed. Per https://github.com/kubernetes/contributor-site/pull/531/files#r1827530755, I don't recommend merging this as-is.

See my comment at https://github.com/kubernetes/contributor-site/pull/531?new_mergebox=true#discussion_r1894993238

My guess is that we need to ensure that the Docsy dependencies are built into the container image (this is what we're planning in https://github.com/kubernetes/website/pull/48812)

The thing is a lot is happening inside the Makefile and hack/gen-content.sh. The Dockefile doesn't even copy over the source and instead the container runs by mounting the source into the container. This is the reason why container-server needs to run npm ci. I personally prefer copying over the source and building inside the container instead of mounting it as that leads to more reproducible builds.

Would you like more advice on how to fix things here @SayakMukhopadhyay? You might find it easiest to bump to 0.5 first and then send in a smaller PR with the bump from 0.5 to 0.10

EDIT: In fact, the big change in Docsy was in 0.7 so upgrading to 0.6 is as trivial as upgrading to 0.5, so maybe, if a separate PR is needed, it should be for up to 0.6.

I was thinking of something similar. I can repurpose this PR to go up to 0.5 and create a new PR for 0.10 (or 0.11) or I can close this PR and create fresh PRs for both. I prefer keeping this PR as a lot of important convo is here.

PS. Once this is good to merge, would you be willing to squash this down to 1 or 2 commits @SayakMukhopadhyay ? Of course.

SayakMukhopadhyay avatar Dec 22 '24 16:12 SayakMukhopadhyay

I personally prefer copying over the source and building inside the container instead of mounting it as that leads to more reproducible builds.

It sounds like you recommend copying the entire website source code into the container image before previewing(?)

I'm not sure if you've tried contributing that way, but it'd be really unenjoyable. The idea of running a containerized preview is that you can open a local browser, make local edits and see changes reflected almost immediately.

If you instead had to quit Hugo, build a new image, and start a new server, this isn't going to feel worthwhile.

For a preview, reproducibility isn't particularly important. The canonical build environment is Netlify; we could use netlify dev for that, but it adds a barrier to entry. Lots of people have a local container runtime; fewer people have netlify.

sftim avatar Dec 22 '24 18:12 sftim

I can repurpose this PR to go up to 0.5 and create a new PR for 0.10 (or 0.11)

Let's repurpose this one.

I can close this PR and create fresh PRs for both. I prefer keeping this PR as a lot of important convo is here.

I agree, let's keep it.

sftim avatar Dec 22 '24 18:12 sftim

I'm not sure if you've tried contributing that way, but it'd be really unenjoyable. The idea of running a containerized preview is that you can open a local browser, make local edits and see changes reflected almost immediately.

If you instead had to quit Hugo, build a new image, and start a new server, this isn't going to feel worthwhile.

For a preview, reproducibility isn't particularly important. The canonical build environment is Netlify; we could use netlify dev for that, but it adds a barrier to entry. Lots of people have a local container runtime; fewer people have netlify.

Ugh, you are right. I generally use other ways like dev envs to quickly integrate but build in container. I don't mind the current setup at all.

SayakMukhopadhyay avatar Dec 22 '24 18:12 SayakMukhopadhyay

If people want to run hugo locally, they can. Or they can preview in a container - as a project, we like containers. CI builds and production deploys are in Netlify and we've no plans to change that.

sftim avatar Dec 22 '24 18:12 sftim

/retitle Upgrade website to Docsy 0.5.1

SayakMukhopadhyay avatar Dec 23 '24 17:12 SayakMukhopadhyay

@SayakMukhopadhyay: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle Upgrade website to Docsy 0.5.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot avatar Dec 23 '24 17:12 k8s-ci-robot

Side note: the value of $gray-800 used to be #888 but was updated to #797676 to improve accessibility. This means that even though _variables.scss in the project would be different from the _variables.scss, it's only becuase it wasn't updated. See google/docsy#540

SayakMukhopadhyay avatar Dec 23 '24 18:12 SayakMukhopadhyay

@sftim This PR has been modified to upgrade only upto 0.5.1. This PR also has an update to the Makefile to ensure that the docsy dependencies are installed on make commands. Once this PR is merged, I will work on #557 which would make things a bit simpler as far as the Makefile is concerned.

I can squash this PR down to a commit if you want. Let me know.

SayakMukhopadhyay avatar Dec 23 '24 18:12 SayakMukhopadhyay

The container image doesn't build for me:

STEP 5/8: RUN npm ci
npm ERR! code EUSAGE
npm ERR!
npm ERR! The `npm ci` command can only install with an existing package-lock.json or
npm ERR! npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or
npm ERR! later to generate a package-lock.json file, then try again.
npm ERR!
npm ERR! Clean install a project
npm ERR!
npm ERR! Usage:
npm ERR! npm ci
npm ERR!
npm ERR! Options:
npm ERR! [--install-strategy <hoisted|nested|shallow|linked>] [--legacy-bundling]
npm ERR! [--global-style] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
npm ERR! [--include <prod|dev|optional|peer> [--include <prod|dev|optional|peer> ...]]
npm ERR! [--strict-peer-deps] [--foreground-scripts] [--ignore-scripts] [--no-audit]
npm ERR! [--no-bin-links] [--no-fund] [--dry-run]
npm ERR! [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
npm ERR! [-ws|--workspaces] [--include-workspace-root] [--install-links]
npm ERR!
npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
npm ERR!
npm ERR! Run "npm help ci" for more info

npm ERR! A complete log of this run can be found in: /root/.npm/_logs/2024-12-24T12_50_27_990Z-debug-0.log
Error: building at STEP "RUN npm ci": while running runtime: exit status 1
make: *** [Makefile:55: container-image] Error 1

Try cloning your repo (you can pull from a filesystem clone to save bandwidth) and check that a container image build runs OK from that clean clone @SayakMukhopadhyay

Once it does, feel free to squash to 1 or 2 commits.

sftim avatar Dec 24 '24 12:12 sftim

@sftim would it be ok, if I COPY over the package.json and package-lock.json over to the Dockerfile. I could do npm i autoprefixer ... again but I think its redundant to list down the required packages in the Dockerfile when they are already there in the package.json.

SayakMukhopadhyay avatar Dec 24 '24 13:12 SayakMukhopadhyay

would it be ok, if I COPY over the package.json and package-lock.json over to the Dockerfile.

Sure, that's a really common technique for this situation.

sftim avatar Dec 24 '24 13:12 sftim

@sftim It's fixed. Do you think there is any value in checking in a CI step that the container builds and runs?

SayakMukhopadhyay avatar Dec 24 '24 13:12 SayakMukhopadhyay

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.

sftim avatar Dec 24 '24 13:12 sftim

Please squash this down to fewer commits; I think that's tidiest.

sftim avatar Dec 24 '24 13:12 sftim

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.

SayakMukhopadhyay avatar Dec 24 '24 15:12 SayakMukhopadhyay

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 avatar Jan 03 '25 10:01 sftim

@sftim as discussed in Slack, the issue is due to the --user : argument which is forcing the container to run as root and thus causing some permission issues. The PR #421 adds the --user : argument probably due to some issue with podman. The website doesn't use the --user argument and it runs fine.

I have also tried running podman myself without the --user : argument and it works with the latest podman so I believe whatever the issue was has been solved by podman.

I have added a commit which removes --user : so could you please try once again.

P.S. we should add a CI job to build and test run the container.

SayakMukhopadhyay avatar Jan 03 '25 19:01 SayakMukhopadhyay