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

Add flag to exit with error instead of asking for interaction #712

Merged
merged 18 commits into from
Aug 25, 2024

Conversation

Rexios80
Copy link
Contributor

@Rexios80 Rexios80 commented Apr 12, 2024

Fixes #710

Copy link

vercel bot commented Apr 12, 2024

@Rexios80 is attempting to deploy a commit to the FlutterTools Team on Vercel.

A member of the Team first needs to authorize it.

@leoafarias
Copy link
Owner

@Rexios80 Shouldn't this return a selection or default selection instead of just exiting? Also, we have added an isCI flag. Would it be the same type of behavior to essentially skip selections in this case or select the default, and if no default is set, then exit?

@Rexios80
Copy link
Contributor Author

The main user input I have issue with is the confirmation to install. The default value for that one is true which is not what I want. It also exits with a success exit code if you say no which is bad since it hides the issue from my tool. As for the selections, I don't see a way to do that without user interaction. The two selections I see (the one for selecting a flutter version and the one for fixing "invalid" flutter versions) both cannot be handled with a default. I mostly added the exit to the select call for consistency with the confirm call, so maybe we can remove that change?

The isCI flag looks promising, but this tool isn't only meant for use in CI so that seems a little weird.

@Rexios80
Copy link
Contributor Author

I think I misread that you wanted to use the isCI flag for this. Yes this PR should be replicating the same behavior that we would expect in CI.

@leoafarias
Copy link
Owner

@Rexios80 can I close this in favor of #713?

@Rexios80
Copy link
Contributor Author

What do you mean "in favor of"? They're two completely separate PRs.

@leoafarias
Copy link
Owner

The force flag also skips interaction. Which command do you need this no interaction for?

@Rexios80
Copy link
Contributor Author

Theoretically it could be any command that asks for interaction, but the biggest issue is when fvm asks to install a flutter version with fvm flutter ...

@Rexios80
Copy link
Contributor Author

@leoafarias What do we need to get this functionality in?

Copy link

vercel bot commented May 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fvm ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 29, 2024 1:05pm

@leoafarias
Copy link
Owner

@Rexios80 can you just update the docs on the website to document this flag?

@Rexios80
Copy link
Contributor Author

Rexios80 commented May 7, 2024

@leoafarias Before I do that are we sure we like the flag name "--no-interact"? I didn't put much thought into it and I don't want us to be stuck with something we don't like forever.

@Rexios80
Copy link
Contributor Author

Rexios80 commented May 7, 2024

Maybe "--no-input" would be better? Or even something like "--fvm-no-input" to guarantee it can't conflict with any dart/flutter flags?

@Rexios80
Copy link
Contributor Author

@leoafarias Please review this when you get a chance

@Rexios80
Copy link
Contributor Author

@leoafarias Do you need anything from me on this?

@leoafarias
Copy link
Owner

@Rexios80 I will bring this in, but will change the default behavior to if isCI, to just use the default value on confirmation... and will define the default values for each.

@leoafarias leoafarias merged commit ec45f17 into leoafarias:main Aug 25, 2024
3 of 4 checks passed
@kaj777
Copy link

kaj777 commented Aug 26, 2024

--fvm-skip-input option is not found. how is the usage? @leoafarias

@leoafarias
Copy link
Owner

@kaj777, there was an issue with the merge. I am doing a hotfix now.

@Rexios80
Copy link
Contributor Author

@leoafarias I appreciate you merging this, but I would strongly prefer the default to be NOT installing a missing flutter version when --fvm-skip-input is passed. If a command is running headless, this will effectively freeze the command for up to several minutes.

@@ -75,10 +75,11 @@ Future<CacheFlutterVersion> ensureCacheWorkflow(
final shouldInstallConfirmed = logger.confirm(
'Would you like to install it now?',
defaultValue: true,
ciDefaultValue: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the reason I added this

@leoafarias
Copy link
Owner

I understand. As you can imagine, I am trying to minimize the flags and flows as much as possible.

What is the case where you run an fvm command that needs a version installed, but you don't want it to install?

@Rexios80
Copy link
Contributor Author

The puby tool can run pub commands in mono-repos in parallel. If a project is set up with fvm, it will call fvm flutter commands. In the case that a user of puby calls puby {link, clean, etc} in an fvm project, but they do not have the required flutter version installed, the command will seem to hang for a long time. It also might try to install flutter many times at once which could cause issues. Since the commands are running in parallel, there is no good solution besides exiting. This could happen when another developer on the project updates the fvm flutter version, but other user's haven't installed it yet.

I don't see a use-case for --fvm-skip-input and auto-installing flutter versions. Scripts running in CI environments should be calling fvm install first anyways.

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.

[Feature Request] Option to error and exit instead of requesting user input
3 participants