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

Firmware releases cleanup #80

Closed
wants to merge 2 commits into from
Closed

Conversation

komret
Copy link
Contributor

@komret komret commented Jun 11, 2024

The first commit removes firmware binaries from directories using the old names 1 and 2. As I understand it, those exist for backward compatibility of trezorctl. However, releases.json could just point to the new directories to avoid duplication, as suggested in #77 (comment). There is still some amount of duplication because the same releases.json exist in both directories - maybe it is feasible to redirect from one URL to the other?

The second commit removes the min_bridge_version property from releases.json. We don't use this property in Suite, perhaps it is not used anywhere else either. Not sure about this one.

@komret komret requested a review from matejcik June 11, 2024 17:25
@komret komret mentioned this pull request Jun 11, 2024
@mmilata
Copy link
Member

mmilata commented Jun 11, 2024

Can we symlink it? It is my understanding that AWS S3 where this is hosted doesn't support symbolic links but aws-cli ought to follow them and create two copies. Not tested.

@komret
Copy link
Contributor Author

komret commented Jun 11, 2024

Can we symlink it? It is my understanding that AWS S3 where this is hosted doesn't support symbolic links but aws-cli ought to follow them and create two copies. Not tested.

question for @vdovhanych :)

@prusnak
Copy link
Member

prusnak commented Jun 11, 2024

I don't see how this commit improves anything but it can break many things.

I suggest not touching something that is not broken.

NACK

Copy link
Member

@vdovhanych vdovhanych left a comment

Choose a reason for hiding this comment

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

I'm with @prusnak on this one. I see the benefit of putting only one file here, but the releases.json is a bit different, and it can break something. Depending on what actually consumes these files, if it will work for that, then why not? We just need to make sure it's not breaking anything.

@matejcik
Copy link
Contributor

to be good citizens of the internet, I would propose the following:

(1) We keep existing binaries in /1/ and /2/ as-is.

This ensures that if someone for whatever reason hardcoded a URL to that file, the URL stays valid

(2) De-duplicate releases.json.

There are two options:

(2a) Stop updating /1/releases.json and /2/releases.json. They are stuck at the current version. Consumers of the files at the old paths (e.g. old trezorctl) will stop receiving firmware updates. File format remains unchanged.

(2b) Make /1/releases.json a copy of /t1b1/releases.json, dtto for t2t1. Remove min_bridge_version. Do not modify paths to binaries, so /2/releases.json will point to /t2b1/firmware-bin.
Consumers of the files at the old paths will continue to receive firmware updates. We rely on them to correctly handle the path that is not in the current directory. We drop a property, meaning something somewhere could break (we don't know what though -- trezorctl never used the "min bridge version" field, Suite isn't using it either)

(as a dispreferred option, (2c) keep the property as a legacy cruft so that old & weird consumers are less likely to break.)

FWIW, at some point we'll want to change the format of releases.json in a more breaking manner anyway, and old consumers will either break or stop receiving updates. I claim that it is better to break old consumers noisily than to keep them un-updated silently.
We don't need to do this change today, but we might as well.

(3) Going forward, do not put binaries in /1 and /2

If we go with (2b) above, we still copy releases.json to the old directories.


The ideal solution, of course, is to make the whole /1 and /2 trees 301 Moved permanently to the respective modern variants. We can't do that today but I understand that if we made a lambda, it could be done, and that there aren't that many requests to these paths anyway.


Speaking of, @vdovhanych, could you look into the logs to see who has downloaded things from /1 and /2 in the last six months? What user agents, and referrers if we have them?

@komret
Copy link
Contributor Author

komret commented Jun 26, 2024

I am closing this because there is not enough support/reasoning to remove the old data and risk breaking something. I talked to @vdovhanych about the logs and retrieving them would cost more time than I can justify so we'll just leave it as is. For future updates, we can follow @matejcik's recommendations 1, 2a (but maybe we keep updating the JSONs?) and 3.

@komret komret closed this Jun 26, 2024
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.

5 participants