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.

Sign up for GitHub

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

Jump to bottom

Platform specific appId #1952

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

Platform specific appId #1952

wants to merge 7 commits into from
+306 −97

Conversation

tokou
Copy link
Contributor

@tokou tokou commented Aug 26, 2024

Proposed changes

  • Introduces MaestroAppId model containing appIds for each platform
  • Tricky parts
    • Ensuring backwards compatibility in deserializing commands
    • Finding a way to have only the current appId in descriptions without passing around Platform too deeply

Testing

  • Added integration tests
  • Ran a flow locally and inspected logs and console output

Issues fixed

Fixes #1472

@tokou
Copy link
Contributor Author

tokou commented Aug 31, 2024

The E2E test works fine for me locally. Maybe we should retrigger it? 🤔

@bartekpacia bartekpacia added the needs backend approval This PR may interfere with Cloud, so needs to be reviewed by a backend engineer. label Sep 2, 2024
@Fishbowler
Copy link
Contributor

Code LGTM, but I've not tested it.
I can see real value in this, even in the simple Wikipedia app examples.

@Fishbowler
Copy link
Contributor

Argh, this has been stale too long - @tokou are you able to rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorsmotto igorsmotto Awaiting requested review from igorsmotto

Assignees
No one assigned
Labels
needs backend approval This PR may interfere with Cloud, so needs to be reviewed by a backend engineer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass different, platform-specific appIds in a single flow
3 participants
@tokou @Fishbowler @bartekpacia