Add configuration option for `archive_source` #337

mipedja · 2023-08-27T01:17:55Z

This allows for specifying a custom archive URL
as an alternative approach to Custom Pages.

This allows for specifying a custom archive URL as an alternative approach to Custom Pages.

niklasmohrin · 2023-08-27T21:40:35Z

Thank you for submitting this PR! I can see from your example code that the motivation for this change seems to be to host your own internal pages - cool idea, I agree that it could be useful.

We need to think about #335 here though. A new version of the client spec is apparently about to be released (see tldr-pages/tldr#10148) which will specify where clients (including tealdeer) will look for language specific archives. So I think we should wait until that is all cleared up and then take note in our documentation accordingly. I will hence mark this as draft for now :)

niklasmohrin

Hey @mipedja,

we have been letting this PR sit for a while now, but I think now is a good time to merge this!

I made some small suggestions, if you want we can discuss them. If you (understandably so) don't want to pick up this PR right now, I will add the final updates myself in a couple of days or so :)

As far as the new client spec discussion goes: The main reason for waiting (language specific archives) has been included in the specification. Since we want to support this, we should change our config option here to not point to some.site/tldr/pages.zip but instead just to some.site/tldr/, and then require that the zip files are present as pages.zip / pages.en.zip and so on. You don't have to add much documentation for this now, as the exact wording has to be changed anyways once we switch to downloading language specific archives :)

niklasmohrin · 2025-01-01T23:25:10Z

docs/src/config_updates.md

+URL for the location of tldr pages archive. Default is the main `thdr.sh`
+archive location.


The new client specification has changed the recommended source for pages again, instead of using tldr.sh, the Github releases are preferred now. I would thus change this to

Suggested change

URL for the location of tldr pages archive. Default is the main `thdr.sh`

archive location.

URL for the location of tldr pages archive. By default the pages are fetched from the latest `tldr-pages/tldr` GitHub release.

niklasmohrin · 2025-01-01T23:25:39Z

docs/src/config_updates.md

+    auto_update = true
+    auto_update_interval_hours = 24


I think we should omit these here, because they are independent of the archive_url option

Suggested change

auto_update = true

auto_update_interval_hours = 24

niklasmohrin · 2025-01-01T23:26:55Z

docs/src/config_updates.md

+    [updates]
+    auto_update = true
+    auto_update_interval_hours = 24
+    archive_url = https://tldr.infra.local/assets/tldr.zip


Let's go with something more neutral like

Suggested change

archive_url = https://tldr.infra.local/assets/tldr.zip

archive_url = https://my-company.example.com/tldr.zip

niklasmohrin · 2025-01-01T23:31:22Z

src/config.rs

@@ -16,6 +16,7 @@ use crate::types::PathSource;
 pub const CONFIG_FILE_NAME: &str = "config.toml";
 pub const MAX_CACHE_AGE: Duration = Duration::from_secs(2_592_000); // 30 days
 const DEFAULT_UPDATE_INTERVAL_HOURS: u64 = MAX_CACHE_AGE.as_secs() / 3600; // 30 days
+const ARCHIVE_URL: &str = "https://tldr.sh/assets/tldr.zip";


Since this is now only used in default_archive_url, I think we can just inline it there (and also update it to the new recommended URL, which is https://github.com/tldr-pages/tldr/releases/latest/download/tldr.zip)

niklasmohrin · 2025-01-02T13:29:57Z

docs/src/config_updates.md

@@ -24,3 +24,13 @@ is set to `false`.
    auto_update = true
    auto_update_interval_hours = 24

+### archive_url


Given that we will soon switch to using a URL where we append pages.en.zip, pages.de.zip etc. ourselves, maybe we should prefer something more neutral as the name. How do you feel about "archive_source"?

mipedja · 2025-01-09T06:20:54Z

Thanks for the review. I agree with all your comments.

I started switching from archive_url to archive_source and then realized you have #345 with archives_url.
Should I just make this to be archives_url instead and then it will fit with #345 well?

For the time being I would hard-code update() to always append tldr.zip, and #345 would make the appropriate language pattern changes.

niklasmohrin · 2025-01-09T13:19:33Z

Very cool that you are coming back to this after the long time waiting for us!

You can disregard the details of #345, I have been working on a rewrite on a separate branch anyways and I am trying to push out smaller changes like #400 first. At some point, I will update #345 or make a new PR.

I think archive_source is better than archives_url, what do you think?

For this PR, it would be okay to have archive_source be the URL to the one zip and change it to be the directory later. However, I suppose if we change it to mean the directory and manually add "tldr.zip" like you suggest, we would need fewer changes in the following PR. So either way is fine, while I slightly prefer your proposed change of always adding "tldr.zip" in the update method :)

niklasmohrin

One more thing I spotted :)

niklasmohrin · 2025-01-09T22:16:45Z

docs/src/config_updates.md

+URL for the location of tldr pages archive. By default is the pages are
+fetched from the latest `tldr-pages/thdr` GitHub release.


Suggested change

URL for the location of tldr pages archive. By default is the pages are

fetched from the latest `tldr-pages/thdr` GitHub release.

URL for the location of the tldr pages archive. By default the pages are

fetched from the latest `tldr-pages/tldr` GitHub release.

niklasmohrin

Very cool, thanks!

niklasmohrin · 2025-01-12T11:07:56Z

src/cache.rs

        self.ensure_cache_dir_exists()?;

+        let archive_url = format!("{}{}", archive_source, "tldr.zip");


I think we can be more cautious here and add the /. If is not present in archive_source, the archive_url will be unexpected, and if it was already present, having // does not change the behavior

Suggested change

let archive_url = format!("{}{}", archive_source, "tldr.zip");

let archive_url = format!("{}/tldr.zip", archive_source);

docs/src/config_updates.md

Add configuration option for archive_url

8daabd2

This allows for specifying a custom archive URL as an alternative approach to Custom Pages.

niklasmohrin marked this pull request as draft August 27, 2023 21:40

dbrgn added the blocked label Feb 11, 2024

niklasmohrin removed the blocked label Jan 1, 2025

niklasmohrin reviewed Jan 1, 2025

View reviewed changes

niklasmohrin force-pushed the custom-archive-url branch from c4e2fd5 to 8daabd2 Compare January 1, 2025 23:52

niklasmohrin mentioned this pull request Jan 2, 2025

Reduce --update invocations in tests #390

Open

niklasmohrin reviewed Jan 2, 2025

View reviewed changes

Update default URL and the wording in documentation

861c092

niklasmohrin reviewed Jan 9, 2025

View reviewed changes

niklasmohrin linked an issue Jan 9, 2025 that may be closed by this pull request

Update source for page cache updates #405

Closed

niklasmohrin added this to the v1.8.0 milestone Jan 9, 2025

Use the path to the archive in archive_source

3451148

mipedja marked this pull request as ready for review January 10, 2025 06:46

niklasmohrin approved these changes Jan 12, 2025

View reviewed changes

niklasmohrin added 3 commits January 12, 2025 12:11

Update src/cache.rs

b73e267

Update docs/src/config_updates.md

fda0103

Merge branch 'main' into custom-archive-url

d22ea3a

niklasmohrin changed the title ~~Add configuration option for archive_url~~ Add configuration option for archive_source Jan 12, 2025

niklasmohrin merged commit 120e2a9 into tealdeer-rs:main Jan 12, 2025
9 checks passed

mipedja deleted the custom-archive-url branch January 12, 2025 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration option for `archive_source` #337

Add configuration option for `archive_source` #337

mipedja commented Aug 27, 2023

niklasmohrin commented Aug 27, 2023

niklasmohrin left a comment

niklasmohrin Jan 1, 2025

niklasmohrin Jan 1, 2025

niklasmohrin Jan 1, 2025

niklasmohrin Jan 1, 2025

niklasmohrin Jan 2, 2025

mipedja commented Jan 9, 2025

niklasmohrin commented Jan 9, 2025

niklasmohrin left a comment

niklasmohrin Jan 9, 2025

niklasmohrin left a comment

niklasmohrin Jan 12, 2025

		URL for the location of tldr pages archive. Default is the main `thdr.sh`
		archive location.

	archive_url = https://tldr.infra.local/assets/tldr.zip
	archive_url = https://my-company.example.com/tldr.zip

		URL for the location of tldr pages archive. By default is the pages are
		fetched from the latest `tldr-pages/thdr` GitHub release.

		self.ensure_cache_dir_exists()?;

		let archive_url = format!("{}{}", archive_source, "tldr.zip");

	let archive_url = format!("{}{}", archive_source, "tldr.zip");
	let archive_url = format!("{}/tldr.zip", archive_source);

Add configuration option for archive_source #337

Add configuration option for archive_source #337

Conversation

mipedja commented Aug 27, 2023

niklasmohrin commented Aug 27, 2023

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin Jan 1, 2025

Choose a reason for hiding this comment

niklasmohrin Jan 1, 2025

Choose a reason for hiding this comment

niklasmohrin Jan 1, 2025

Choose a reason for hiding this comment

niklasmohrin Jan 1, 2025

Choose a reason for hiding this comment

niklasmohrin Jan 2, 2025

Choose a reason for hiding this comment

mipedja commented Jan 9, 2025

niklasmohrin commented Jan 9, 2025

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin Jan 9, 2025

Choose a reason for hiding this comment

niklasmohrin left a comment

Choose a reason for hiding this comment

niklasmohrin Jan 12, 2025

Choose a reason for hiding this comment

Add configuration option for `archive_source` #337

Add configuration option for `archive_source` #337