Skip to content

Fix: Site BaseURLs #16568

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

Merged
merged 11 commits into from
Jul 14, 2025
Merged

Fix: Site BaseURLs #16568

merged 11 commits into from
Jul 14, 2025

Conversation

Ajay-singh1
Copy link
Contributor

@Ajay-singh1 Ajay-singh1 commented Jun 9, 2025

Description

Related Issue:-#16559

This PR fixes incorrect baseURL values used during site generation, which were causing invalid or broken URLs in sitemap.xml.

Summary of changes

Added logic for baseURLs based on different netlify contexts.

How can we check if the baseURLs are pointing correctly?

  • Really can`t tell until the PR is merged into the master branch.

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@Ajay-singh1 Ajay-singh1 requested a review from a team as a code owner June 9, 2025 09:26
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test labels Jun 9, 2025
@istio-testing
Copy link
Contributor

Hi @Ajay-singh1. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@craigbox
Copy link
Contributor

craigbox commented Jun 9, 2025

Can you ping me on the Istio Slack please Ajay?

@craigbox
Copy link
Contributor

craigbox commented Jun 9, 2025

/ok-to-test

(I think you can join as a member now btw)

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jun 9, 2025
@Ajay-singh1 Ajay-singh1 force-pushed the fix-baseURL branch 2 times, most recently from 0212a07 to 2c735ca Compare June 9, 2025 11:14
@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 9, 2025
@Ajay-singh1 Ajay-singh1 force-pushed the fix-baseURL branch 2 times, most recently from de2a06d to b7ca906 Compare June 9, 2025 13:22
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 9, 2025
@Ajay-singh1 Ajay-singh1 changed the title Test:Fix BaseURLs Fix: SIte BaseURLs Jun 9, 2025
@Ajay-singh1 Ajay-singh1 changed the title Fix: SIte BaseURLs Fix: Site BaseURLs Jun 9, 2025
@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 10, 2025
@Ajay-singh1 Ajay-singh1 force-pushed the fix-baseURL branch 2 times, most recently from 38adea3 to 7d41c93 Compare June 11, 2025 07:36
@Ajay-singh1
Copy link
Contributor Author

@craigbox I have got a working solution now.We can check the sitemap for preliminary deployment and it is generating those sitemaps correctly.See :- Sitemap for Preliminary deployment.

@Ajay-singh1 Ajay-singh1 requested a review from craigbox June 11, 2025 08:34
Co-authored by: Craig Box <[email protected]>
@Ajay-singh1
Copy link
Contributor Author

Ajay-singh1 commented Jul 9, 2025

@craigbox We are very close! Fully qualified URLs from the deployment-preview.
Attached PNG for reference:-

Screenshot from 2025-07-09 14-28-02

@Ajay-singh1
Copy link
Contributor Author

This will work correctly if the environment variables are set correctly in netlify.

@craigbox
Copy link
Contributor

craigbox commented Jul 9, 2025

https://docs.netlify.com/configure-builds/environment-variables/#deploy-urls-and-metadata

URL: URL representing the main address to your site. It can be either a Netlify subdomain or your own custom domain if you set one; for example, https://petsof.netlify.app/ or https://www.petsofnetlify.com/.

Primary domains for the three sites are: istio.io, preliminary.istio.io, istio-staging.netlify.app

@Ajay-singh1
Copy link
Contributor Author

Ajay-singh1 commented Jul 10, 2025

Thanks! Craig , Let's merge this PR now.This will work 😀.

When on https://istio.io the sitemap will be generated like https://istio.io/latest/sitemap.xml

When on preliminary.istio.io the sitemap will be generated like https://preliminary.istio.io/latest/en/sitemap.xml

When on istio-staging.netlify.app the sitemap will be generated like
https://istio-staging.netlify.app/latest/en/sitemap.xml

When ondeploy-previews the sitemap will be generated like this
https://$(deploy-preview-url)/latest/en/sitemap.xml

All the URLs will be set correctly and Hugo will use this different baseURLs when building and deploying the site.

@Ajay-singh1 Ajay-singh1 requested review from dhawton and craigbox July 10, 2025 08:09
@Ajay-singh1 Ajay-singh1 added the cherrypick/release-1.26 Set this label on a PR to auto-merge it to the release-1.26 branch label Jul 11, 2025
Makefile.core.mk Outdated
@@ -72,10 +72,20 @@ JUNIT_REPORT := $(shell which go-junit-report 2> /dev/null || echo "${ISTIO_BIN}
ISTIO_SERVE_DOMAIN ?= localhost
export ISTIO_SERVE_DOMAIN

# Determine the baseURL for the site depending on the context
Copy link
Contributor

Choose a reason for hiding this comment

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

I went back and forward on deciding if I should suggest you reuse ISTIO_SERVE_DOMAIN here. Probably not, because the below would then override people who are trying to set the domain for running make serve.

(Then I went back to this comment and changed to the suggestion below)

(Then I redid it all a third time)

I've also tried to figure out how we can get /latest into the site config for baseURL. When you run hugo serve (not make serve) it doesn't use /latest/ in the URL; you have to specify that yourself. (I get bitten by this a lot because I run a local hugo, not the containerised version that comes with make serve.)

Right now I don't think it can be done, because the baseURL that you set in hugo.toml is for the production environment, but the development environment (as used by hugo serve) doesn't obey that value. I'll open an issue on the Hugo issue tracker for that.

Instead, I'm going to change the variable name to match the format used in the rest of the file.

@craigbox
Copy link
Contributor

deploy preview log:

1:42:21 PM: + hugo --minify --baseURL https://deploy-preview-16568--preliminary-istio.netlify.app/latest

@istio-testing istio-testing merged commit c4a8f5f into istio:master Jul 14, 2025
6 checks passed
@istio-testing
Copy link
Contributor

In response to a cherrypick label: #16568 failed to apply on top of branch "release-1.26":

Applying: Fix: Site baseURLs
.git/rebase-apply/patch:38: trailing whitespace.
	    [email protected] 
.git/rebase-apply/patch:44: trailing whitespace.
	@scripts/build_site.sh "${baseurl}"   
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	Makefile.core.mk
Falling back to patching base and 3-way merge...
Auto-merging Makefile.core.mk
CONFLICT (content): Merge conflict in Makefile.core.mk
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix: Site baseURLs

@istio-testing
Copy link
Contributor

In response to a cherrypick label: new issue created for failed cherrypick: #16661

@craigbox
Copy link
Contributor

preliminary log:

1:50:50 PM: + hugo --minify --baseURL https://preliminary.istio.io/latest

@Ajay-singh1 Ajay-singh1 deleted the fix-baseURL branch July 14, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick/release-1.26 Set this label on a PR to auto-merge it to the release-1.26 branch ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants