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

Minimal Hyperion Version #480

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Minimal Hyperion Version #480

wants to merge 9 commits into from

Conversation

warix8
Copy link

@warix8 warix8 commented Feb 4, 2025

These changes introduce a minimal Hyperion version to ensure Titan can work properly.

When a new Titan version with a new Hyperion including breaking changes (e.g. login system) is introduced, there will be a downtime because the stores take a few hours to review the version.

Thus, the new version is submitted to the store, this version will work because it will fall back to the alpha version.

No downtime anymore for introducing breaking changes.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 53.71%. Comparing base (5a5499f) to head (9d8e9f7).

Files with missing lines Patch % Lines
lib/tools/functions.dart 0.00% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
- Coverage   54.18%   53.71%   -0.47%     
==========================================
  Files         169      169              
  Lines        3739     3764      +25     
==========================================
- Hits         2026     2022       -4     
- Misses       1713     1742      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Marc-Andrieu
Copy link
Contributor

To explain that in a nutshell: it's the Titan counterpart of Hyperion config's MINIMAL_TITAN_VERSION_CODE.

On start (with the prod flavor), Titan will ensure this minimal version is inferior to the version of the prod Hyperion. If not, it will change Repository.host to the alpha Hyperion.

Copy link
Member

@maximeroucher maximeroucher left a comment

Choose a reason for hiding this comment

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

This is an interesting and mandatory work.
Nice one for a first !!

I am considering another option, which is to make a provider storing the host, and the repository listening to this value when making repositoryProviders.
It will not require the GlobalData class and support dependency injection, which sounds better to me.
I will be glad to discuss this with you both !

var host = dotenv.env["${getAppFlavor().toUpperCase()}_HOST"];

if (host == null || host == "") {
throw StateError("Could not find host corresponding to flavor");
}

// Get backend version
final response = await http.get(Uri.parse("$host/information"));
Copy link
Member

Choose a reason for hiding this comment

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

I may be mistaking, but I am under the impression we also make this request to check minimal_titan_version. Could we combine the two verifications to only make one request?

toDecode = utf8.decode(response.body.runes.toList());
var decoded = jsonDecode(toDecode);
final version = decoded.version;
final minimalHyperionVersion = "4.0.0";
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this constant in a more convenient place? Maybe the pubspec if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was in a hurry when writing this, I made by analogy with Hyperion which has the version in a config.py.

In this analogy, I don't really see a more relevant file than this one to write.

Copy link
Member

Choose a reason for hiding this comment

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

This is indeed in config.py which is the only hard-coded configuration file of Hyperion. For Titan the version and build number are already in the pubspec

@Marc-Andrieu
Copy link
Contributor

Marc-Andrieu commented Feb 18, 2025

There are 3 cases to consider (and test, assuming the flavor is dev), and checking in this list only if the test has passed:

  • The flavor host is compatible
  • The flavor host is not compatible but the alpha host is
  • Neither is compatible

(that is, I tested the 3 cases and Titan is not stuck only in the 1st case so far)

@warix8 warix8 added the ready for review This PR is ready to be reviewed label Feb 20, 2025
@Marc-Andrieu
Copy link
Contributor

Note

Update: now I can confirm it works in all cases.

IMO there remain 3 things to consider:

  1. What UI to display when neither host is compatible? Currently it just seems to be loading. A page for this in the others module would be nice.
  2. Could we use the repository in the version module to send and process the request? So we don't reinvent the wheel and have a shorter yet more secure code.
  3. Where to write the minimal Hyperion version? As a reminder, Hyperion's minimal Titan version is in app/core/config.py. not in a yaml, toml, txt, etc., closer to the root.

String getTitanHost() {
var host = dotenv.env["${getAppFlavor().toUpperCase()}_HOST"];
Future<String> getTitanHost() async {
const String minimalHyperionVersion = "4.0.0";
Copy link
Member

Choose a reason for hiding this comment

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

Move this declaration in pubspec.yaml as it is the file where all versions are declared

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants