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

Include final URL #157

Merged
merged 1 commit into from Sep 10, 2020
Merged

Include final URL #157

merged 1 commit into from Sep 10, 2020

Conversation

somini
Copy link
Contributor

@somini somini commented Apr 25, 2020

The site is available on that particular URL, so include it on the URL configuration.

This should fix the errors I get on the feed generation.

https://validator.w3.org/feed/check.cgi?url=https%3A%2F%2Fwww.openttd.org%2Ffeed.xml

The feed itself does not validate, but this is kinda strange, because my feed reader accepts with weird post URLs.

The site is available on that particular URL, so include it on the URL configuration.

This should fix the errors I get on the feed generation.

https://validator.w3.org/feed/check.cgi?url=https%3A%2F%2Fwww.openttd.org%2Ffeed.xml

The feed itself does not validate, but this is kinda strange, because my feed reader accepts with weird post URLs.
@somini
Copy link
Contributor Author

somini commented Apr 25, 2020

Is it possible to see the built feed.xml on this PR, with Github Actions? Gitlab CI allows this, but I can't seem to find an option to do this.

@TrueBrain
Copy link
Member

No, we currently don't have previews for Pull Requests. And it has little to do with GitHub Actions or GitLab CI. In both cases you have to configure that in order for it to work, and we haven't yet ;)

I will have to validate the effect of your change, but we have been busy releasing BaNaNaS v1.5, so this is a bit delayed. I expect I get around to do this no earlier than next weekend. Sorry about that delay.

@LordAro
Copy link
Member

LordAro commented May 10, 2020

From the jekyll-feed docs:

url - The URL to your site, e.g., https://example.com. If none is provided, the plugin will try to use site.github.url.

@TrueBrain
Copy link
Member

TrueBrain commented May 11, 2020

Tnx for the reminder @LordAro ; kinda forgot about this Pull Request (SORRY!!).

The main issue we have: this Docker runs both staging as production. They have different URLs. So defining an URL here is a bit annoying, as that leads to faulty links (those that exist on staging but not on production, for example). Basically, the real URL is just a result of the infra; and this container should not care.

This is why url is empty; at build-time, there is no value that is correct.

As you mention, readers accept the feed just fine; it is against specs, but it works fine. So I wonder if this is worth fixing (honest question). If we do, W3C will be happy, but staging will break. If we don't, all readers still accept the feed, just W3C will be unhappy.

To properly fix this, from what I can tell, is a bit of an annoying process: we would have to add a marker in url, like $$URL$$, and sed-replace that on startup with an env-variable. I rather avoid that :D

Thoughts?

@somini
Copy link
Contributor Author

somini commented May 12, 2020

The main reason I created this was because my feed reader did not accept the feed without weird post URL. It's the relative URL that do it. I wouldn't open a PR just for strict standards adhesion 😁 .

I checked the jekyll docs and it seems you can override the baseurl easily on the command line, but not url. 😞

I did however learned something new: Jekyll supports multiple configuration files:

Specify config files instead of using _config.yml automatically. Settings in later files override settings in earlier files.
--config FILE1[,FILE2,...]

So you could keep the current config file, and have two extra files:

  • _config.staging.yml
url: "staging.url"
  • _config.production.yml
url: "production.url"

And run it with --config _config.yml,_config.$MACHINE.yml or similar. Those files are easy enough to be generated during the build with a simple "echo".

This is a low-priority thing, so feel free to just close this as "too much of a hassle".

@TrueBrain
Copy link
Member

Ah, sorry, I misread and thought it wasn't a real issue :D My bad!

Sadly, we have to change the URL on run-time, not on build-time. The container running on staging and production is the same (as otherwise you are not really testing anything on staging after all ;) ), so it cannot be some configuration in Jekyll.

Ideas are welcome, but I am tempted to do a sed. There are more placed that can benefit from this, so it might in general just be a good idea.

@somini
Copy link
Contributor Author

somini commented May 13, 2020

I see. The "supported" way is to do two builds, changing only the url.

Sadly, we have to change the URL on run-time, not on build-time. The container running on staging and production is the same (as otherwise you are not really testing anything on staging after all ;) ), so it cannot be some configuration in Jekyll.

Maybe I'm misunderstanding, but if you are running sed anyway, doesn't this mean you need two different containers anyway, for staging and production?

@TrueBrain
Copy link
Member

The sed would run with an env-variable which would be set on runtime :)

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

It has been a while, but I was thinking this over again ..

Nobody should have the feed of staging in its reader. It is stupid. So this Pull Request is totally fine, even if the feed on staging points to production. So I am going to merge this PR :)

Sorry for the delay, and thank you for the PR!

@TrueBrain TrueBrain merged commit 38fd927 into OpenTTD:master Sep 10, 2020
@somini
Copy link
Contributor Author

somini commented Sep 12, 2020

This hasn't been deployed yet, right? At least I can't see the new feed yet.

@TrueBrain
Copy link
Member

This is currently on staging: https://www.staging.openttd.org/feed.xml

After the next release, it will move to production :)

(and I now see it has a / too much .. lol)

@somini
Copy link
Contributor Author

somini commented Sep 12, 2020

That's what the staging is for. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants