Skip to content
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

Upgrade Hugo #3369

Merged
merged 11 commits into from
Sep 14, 2023
Merged

Upgrade Hugo #3369

merged 11 commits into from
Sep 14, 2023

Conversation

Kalaiselvi84
Copy link
Contributor

@Kalaiselvi84 Kalaiselvi84 commented Sep 8, 2023

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Work on #3346

Special notes for your reviewer:

@github-actions github-actions bot added the kind/cleanup Refactoring code, fixing up documentation, etc label Sep 8, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3cb31877-3bc3-4e61-a24e-f74cdc1af549

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@markmandel
Copy link
Member

site/docs/third-party-content/videos-and-presentations/index.html
  target does not exist --- site/docs/third-party-content/videos-and-presentations/index.html --> /css/prism.css
  target does not exist --- site/docs/third-party-content/videos-and-presentations/index.html --> /js/prism.js

I wonder why that is? Those files should (in theory) be coming from the docsy theme 🤔

@markmandel
Copy link
Member

When you run make site-server does the prism code syntax highlighting work for you locally?

@Kalaiselvi84
Copy link
Contributor Author

When you run make site-server does the prism code syntax highlighting work for you locally?

both css/prism.css and js/prism.css have 200 OK
https://screenshot.googleplex.com/4H6yJ2reVqYmPSx

https://screenshot.googleplex.com/7JwhtWmTW2WFR6B

Am I answering your question?

@markmandel
Copy link
Member

You are!

If you look at the source code the page, is there more than one reference to a prism.js ?

@Kalaiselvi84
Copy link
Contributor Author

I hope that you are referring to the HTML source code that is present in the Elements tab of the web page.

https://screenshot.googleplex.com/6oKRPcvTgpYunsN

Console tab says unable to load a source map file for the "asciinema-player.js" file
https://screenshot.googleplex.com/4w5iM5dxxeP68j9

@markmandel
Copy link
Member

I hope that you are referring to the HTML source code that is present in the Elements tab of the web page.

Yep! And clicking through to /js/prism.js - that all just works? 🤔 don't know what's up - that's odd it's failing on test.

@Kalaiselvi84
Copy link
Contributor Author

Kalaiselvi84 commented Sep 12, 2023

clicking through to /js/prism.js - that all just works?

yes, the prism.js code is there.
https://screenshot.googleplex.com/9ZYbWNCXbQXc9Dt

@markmandel
Copy link
Member

The only weird idea I have is that htmltest doesn't like a /from/root path like this when testing a folder? But that also seems weird.

Interesting test: On the old version of the website, is the html source for the prism files the same?

@Kalaiselvi84
Copy link
Contributor Author

On the old version of the website, is the html source for the prism files the same?

Yes, I have compared js/prism.js between the main branch and this Hugo feature branch, and there are no differences between these two branches

@markmandel
Copy link
Member

Yes, I have compared js/prism.js between the main branch and this Hugo feature branch, and there are no differences between these two branches

Not the files themselves, but the path to the files in the html source.

If I look at: view-source:https://agones.dev/site/docs/installation/install-agones/yaml/ I see:

<link rel="stylesheet" href="[/site/css/prism.css](https://agones.dev/site/css/prism.css)"/>

So it looks like it's the same.

For the test though we do generate to a local directory as a static copy of the website, so might be worth doing a compare and contrast of what is statically generated.

@Kalaiselvi84
Copy link
Contributor Author

words are disappearing😅. Pasted the differences in the buganizer ticket

@Kalaiselvi84
Copy link
Contributor Author

@markmandel

I've tried including different path configurations like 'css' or 'site,' or omitting these two, and I've used 'relURL' or 'absURL' for the prism reference. However, I'm still encountering a failure with the hugo-test target.

@markmandel
Copy link
Member

Can you push one of the options to this draft PR, so I can have a look?

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Sep 13, 2023
@Kalaiselvi84
Copy link
Contributor Author

I included /site/ and used absURL in this PR.
Hugo-test log - https://gist.github.com/Kalaiselvi84/98e9280c3be7f052a834446a4c7386d6

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 6af478a8-0187-43d3-8919-d8f55adaae33

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't solve all the issues (looks like there are a couple of other broken links with the version upgrade), but this solves the prism issues.

Approach taken from https://github.com/google/docsy/blob/5295589fd575084a17774a0e93a12f5708e9b7c4/layouts/partials/head.html#L41 (latest Docsy theme).

@@ -34,7 +33,7 @@
{{end}}
{{ if .Site.Params.prism_syntax_highlighting }}
<!-- stylesheet for Prism -->
<link rel="stylesheet" href="{{ "/css/prism.css" | relURL }}"/>
<link rel="stylesheet" href="{{ "/site/prism.css" | absURL }}"/>
Copy link
Member

@markmandel markmandel Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<link rel="stylesheet" href="{{ "/site/prism.css" | absURL }}"/>
<link rel="stylesheet" href="{{ "css/prism.css" | relURL }}"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it. hugo-test failed with two errors. Details: https://gist.github.com/Kalaiselvi84/7f12221fa15b8e8707587039fcba59ec

{{- template "_internal/schema.html" . -}}
{{- template "_internal/twitter_cards.html" . -}}
{{ if eq (getenv "HUGO_ENV") "production" }}
{{ template "_internal/google_analytics.html" . }}
{{ end }}
{{ if .Site.Params.prism_syntax_highlighting }}
<!-- stylesheet for Prism -->
<link rel="stylesheet" href="{{ "/css/prism.css" | relURL }}"/>
<link rel="stylesheet" href="{{ "/site/prism.css" | absURL }}"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<link rel="stylesheet" href="{{ "/site/prism.css" | absURL }}"/>
<link rel="stylesheet" href="{{ "css/prism.css" | relURL }}"/>

@Kalaiselvi84
Copy link
Contributor Author

Moved the 404.html file to the 'docsy' folder, and the error related to this file is resolved. Is it acceptable to keep the file in this location?

@Kalaiselvi84
Copy link
Contributor Author

hugo-test is passed. here is the successful log - - https://gist.github.com/Kalaiselvi84/c044264e40b461a5b8db2a7a6f0ebbaf

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 807a95cf-7750-4a8b-aa10-f7f997559099

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3369/head:pr_3369 && git checkout pr_3369
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-0253ae1-amd64

@markmandel
Copy link
Member

Moved the 404.html file to the 'docsy' folder, and the error related to this file is resolved. Is it acceptable to keep the file in this location?

We don't actually use the 404, so I think we can delete it!

https://gohugo.io/templates/404/

This is what a 404 looks like for us: https://agones.dev/site/index3.html

@markmandel
Copy link
Member

Cool cool - let's take a look at the preview when it comes up.

@Kalaiselvi84
Copy link
Contributor Author

The hugo-test, site-server, site-static, and site-static-preview targets have successfully completed, with the total of two warnings.

@markmandel
Copy link
Member

The hugo-test, site-server, site-static, and site-static-preview targets have successfully completed, with the total of two warnings.

Sweet! I would like to give a look to see if anything's broken from a rendering perspective as well.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 015fac38-ec1a-4076-9c27-d559c9b40767

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3369/head:pr_3369 && git checkout pr_3369
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.35.0-dev-9b1c1be-amd64

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking through, this looks good!

Next stop 😄 let's try upgrading Docsy once this is merged.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Kalaiselvi84, markmandel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel markmandel merged commit c8649e2 into googleforgames:main Sep 14, 2023
2 checks passed
@Kalaiselvi84
Copy link
Contributor Author

Next stop 😄 let's try upgrading Docsy once this is merged.

Could you kindly advise on the most appropriate option for our project from the link?

@markmandel
Copy link
Member

I know they don't seem to advocate for it, but let's start with a copy of docsy in our folder. I feel like we often need to make changes to the docsy theme, so having our own copy seems like a good idea.

@Kalaiselvi84 Kalaiselvi84 deleted the hugo branch March 15, 2024 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/cleanup Refactoring code, fixing up documentation, etc lgtm size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants