Make appId optional in flow config#3086
Conversation
Fishbowler
left a comment
There was a problem hiding this comment.
With appIds and URLs required by a number of commands and by Maestro Cloud, and a natural configuration for many environments, I'm not sure that I understand the value of this change well enough to justify the complexity it introduces.
|
@Fishbowler two cases where this matters: Subflows that don't launch or stop anything — a login helper, a checkout flow, anything that just interacts with what's on screen: name: Log in as a temporary user
---
- runScript: createTemporaryAccountViaTestService.js
- tapOn: Email
- inputText: ${output.email}
- tapOn: Password
- inputText: ${output.password}
- tapOn: Sign InToday this needs a dummy You could put a dummy appId there, sure, but normally, if multiple apps share the same screen (login, onboarding) and you want one subflow for it — whose Then there's #1692 — flows that don't target an app at all (deep link testing, web content). There's no appId to provide, not even via env var. name: Flow without appId
---
- openLink: https://example.com
- tapOn: Learn moreRegarding Maestro Cloud — good point, I hadn't considered that. Does Cloud need appId on subflows too, or only on entry point flows? That said, I might be missing the full picture — I noticed appId is used implicitly in some places (e.g. Android settling), and I don't have visibility into what Cloud expects. If this creates more problems than it solves, happy to address concerns or close it if you feel like this absolutely goes against your direction. |
|
Cloud, like Studio, is just a wrapper around Maestro. There's a check on upload that the appId of the top level flows matches the uploaded binary. The usecase in #1692 alone isn't enough to justify this change. That being said, there's now zero mandatory fields. Does your change allow for the first YAML document in a file to be empty? |
So if I understand correctly, I’d basically need to enforce validation that top-level flows must include
@Fishbowler, would it be reasonable to handle it that way? As things stand, I believe there’s still some validation left (something like "when parsed, it must not be an array"). I was also wondering whether removing it would be acceptable. To me, removing it seems reasonable. 🙂 |
Correct.
I think an empty first document is acceptable. I think no first document probably isn't. |
Fixes #1692
Proposed changes
Allow running Maestro flows without specifying
appId(orurl) in the YAML config section.Currently every flow must declare
appIdorurlin its frontmatter, even if the flow only opens a deep link or interacts with whatever app is already on screen. This change moves the validation from config-level to command-level: a flow withoutappIdparses fine, but commands that need one (launchApp,stopApp,killApp,clearState,setPermissions) will throw an error if noappIdis available — either inline or from config.Before — always required, even for flows that don't need it:
After — optional, commands that need it say so:
And if you do need
launchApp, provide it inline:Error when
appIdis missing and a command needs it:What changed
YamlConfig—appIdis now nullable, removed theConfigParseError("missing_app_target")checkYamlFluentCommand— allappId: Stringparams becomeString?, addedrequireAppId()helper for per-command validationMaestroFlowParser— removed deadConfigParseErrorhandler, addedSyntaxErrorcase toToCommandsExceptionhandler for clean error messagesConfigParseErrorclass (no longer thrown)No changes to
Commands.ktmodels,Maestro.kt,Driver.kt, orOrchestra.kt.Testing
openLink(parses), flow without appId + inlinelaunchApp(parses), flow without appId + barelaunchApp(errors)launchAppe018_config_missing_appIdworkspace error test to verify the new per-command error message./gradlew :maestro-orchestra:test :maestro-test:test)