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

CLIENT-SPECIFICATION: update asset URLs to use GitHub releases, disambiguate triple-brace placeholders #12158

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

acuteenvy
Copy link
Member

@acuteenvy acuteenvy added the decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. label Jan 28, 2024
@github-actions github-actions bot added the documentation Issues/PRs modifying the documentation. label Jan 28, 2024
@kbdharun
Copy link
Member

kbdharun commented Feb 4, 2024

Regarding @sbrl's PR at #12133 should we close it and mark it as superseded by this one, or merge it then rebase the current PR to it?

@sbrl
Copy link
Member

sbrl commented Feb 6, 2024

Merge mine and then rebase? Why a second PR?

@kbdharun
Copy link
Member

kbdharun commented Feb 7, 2024

Merge mine and then rebase? Why a second PR?

Feel free to do whichever method you prefer, there seems to be some additional changes here (if you can incorporate it into your PR that works for me too).

@sebastiaanspeck sebastiaanspeck merged commit 6bd54d3 into tldr-pages:main Feb 14, 2024
4 checks passed
@acuteenvy
Copy link
Member Author

acuteenvy commented Feb 14, 2024

We're not releasing the new spec yet, are we? Was there some discussion I missed? I don't think this was supposed to be merged.

@kbdharun
Copy link
Member

kbdharun commented Feb 15, 2024

We're not releasing the new spec yet, are we? Was there some discussion I missed? I don't think this was supposed to be merged.

Yeah, we haven't discussed about merging this yet or how to deal with sbrl's PR. Since it got merged, no need to revert it, I will check what to do later today (if any changes are required will open a new PR or modify Sbrl's).

We aren't going to make a new release yet, as we are discussing something about funding (moving to Open Collective, etc) in the chatroom thread, feel free to join the conversation over there.

sebastiaanspeck pushed a commit to sebastiaanspeck/tldr that referenced this pull request Feb 15, 2024
@acuteenvy acuteenvy deleted the client-spec-v2.2 branch February 16, 2024 17:53
@sbrl
Copy link
Member

sbrl commented Feb 23, 2024

AFAIK funding discussion on Matrix (which should really be repeated in an issue I think) stalled due to UK advertisement law that could hold some maintainers personally liable.

That discussion probably shouldn't block this one or a release, since discussion of any funding etc should probably be discussed in a dedicated release post / etc?

@kbdharun
Copy link
Member

kbdharun commented Feb 23, 2024

AFAIK funding discussion on Matrix (which should really be repeated in an issue I think) stalled due to UK advertisement law that could hold some maintainers personally liable.

That discussion probably shouldn't block this one or a release, since discussion of any funding etc should probably be discussed in a dedicated release post / etc?

Agreed, then IG we can discuss it in a later date (ideally on an issue). Can you check the client specification and see if the changed wording is fine?

I will try to make a release this weekend if everything is good (IG we can go with version 2.5 for now and when the actual change is done [removal of assets from website] we can make a major release 3.0).

@sbrl
Copy link
Member

sbrl commented Feb 23, 2024

Sounds good.

Can you check the client specification and see if the changed wording is fine?

Sure thing! As ofhttps://github.com/tldr-pages/tldr/blob/60640ac3519aa9901177d5bfe7decbbcec8f44cf/CLIENT-SPECIFICATION.md#caching, which is the latest change to CLIENT-SPECIFICATION.md on main as of the time of writing this comment, the language in the Caching section seems fine to me. Been a buncha PRs coming through ref the specs and stuff, so got a bit mixed up as to which PR was merged when haha

I will try to make a release this weekend if everything is good (IG we can go with version 2.5 for now and when the actual change is done [removal of assets from website] we can make a major release 3.0).

Sounds good! Could you make sure to mention in the release at least that the existing URLs for download are NOT going away to assuage any fears from client spec implementors? Might be worth a quick update to the spec changelog too ref this.

Many thanks for all your hard work!

@kbdharun
Copy link
Member

kbdharun commented Feb 23, 2024

Sounds good! Could you make sure to mention in the release at least that the existing URLs for download are NOT going away to assuage any fears from client spec implementors? Might be worth a quick update to the spec changelog too ref this.

Mention that it isn't going away now, but in the future one?

We discussed about continuing the website method but it would still have the storage issue and would introduce additional maintenance. Maybe, we could add a redirect for the main tldr.zip asset alone? [For backward compatibility with clients not updated yet] (To redirect from the website to the GitHub assets link; I haven't tested this one yet so not sure how it would work as it can't be done in the DNS side from Cloudflare so we need to setup a normal HTML redirect using GitHub pages under the assets directory.)

@sbrl
Copy link
Member

sbrl commented Feb 24, 2024

A redirect would work if that works with GitHub pages? Note it would need to generate the appropriate HTTP status code 'cause clients won't parse HTML. Or else just stop updating the zip on the website?

@acuteenvy
Copy link
Member Author

Unfortunately, making 301 redirects with GitHub pages is impossible. The only way to do it is via a redirect meta tag (like in the chatroom repository), which returns 200 OK instead:

$ curl https://tldr.sh/chatroom -Lsw "\nResponse code: %{response_code}\n"
<!DOCTYPE html>
<html lang="en">
<head>
    <title>TLDR Pages Chatroom</title>
    <meta http-equiv="refresh" content="0; url=https://matrix.to/#/#tldr-pages:matrix.org" />
</head>
</html>

Response code: 200

I think the best way to tackle this is to put a notice in the release notes saying that the old URL is now deprecated, and then stop publishing assets after some time. After that we can proceed with the changes to the website repo proposed in #12048 (comment).

@kbdharun
Copy link
Member

kbdharun commented Feb 25, 2024

I think the best way to tackle this is to put a notice in the release notes saying that the old URL is now deprecated, and then stop publishing assets after some time. After that we can proceed with the changes to the website repo proposed in #12048 (comment).

Agreed, I will try to test the meta tag redirect to the GitHub release asset of complete zip alone (since a majority of clients still use it), but I don't think we should advertise this method in release notes and instead keep it just for compatibility with a lot of abandoned clients.


Edit. I initially thought of making a release today, but let's discuss this thoroughly first. So I will postpone the client specification release a few more days IG.

@kbdharun kbdharun added this to the v2.5 milestone Feb 25, 2024
@acuteenvy
Copy link
Member Author

acuteenvy commented Feb 25, 2024

Agreed, I will try to test the meta tag redirect to the GitHub release asset of complete zip alone

As @sbrl said, clients would have to parse HTML for this to work. Just try to send a GET request to https://tldr.sh/chatroom with any HTTP client that isn't a web browser - it will return this instead of redirecting to Matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision A (possibly breaking) decision regarding tldr-pages content, structure, infrastructure, etc. documentation Issues/PRs modifying the documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants