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

Added --non-interactive and --force support when --id is not passed to eas init command #1983

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

mymattcarroll
Copy link
Contributor

Why

We have recently updated to version 3.17.0 of eas-cli (from 1.2.0). The eas init command previously supported running without prompts to either:

  • create a new project within expo if the project did not exist
  • or if the project did exist in expo, add the associated id to the app.json file as extra.eas.projectId.

The version 3.17.0 eas init command does support --non-interactive and --force but not for the two use cases above.

How

I added support the --force and --non-interactive to the following two prompts (ids and names are just examples):

Existing project found: accountname/projectname (ID: 73b72c6f-76d5-4728-9dc7-d9f6633c0d4e). Link this project?
Would you like to create a project for accountname/projectname?

Test Plan

Untested, open to feedback before continuing with implementation.

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

Hi 👋

Thank you very much for contributing 👏 ! Sorry that you waited so long for the response 🙏.

It generally looks good 🚀! I just tested it locally and it seems like it won't work in its current form because

force: Flags.boolean({
description: 'Whether to overwrite any existing project ID',
dependsOn: ['id'],
}),
// this is the same as EASNonInteractiveFlag but with the dependsOn
'non-interactive': Flags.boolean({
description: 'Run the command in non-interactive mode.',
dependsOn: ['id'],
}),

the force and non-interactive flags depend on the id flag. So currently you won't be able to use force and non-interactive without the id flag specified.

Are you still interested in working on this PR? If so, I can provide you with some guidance:

  • https://github.com/expo/eas-cli/blob/main/CONTRIBUTING.md - Here are some instructions regarding how you can run EAS CLI locally to test it, it will be easier for you if you can actually test it 😄
  • The next thing to do is to delete this flag dependency. I hope that after this is done everything will work fine! 🚀

Let me know if you have any more questions or need some help!

Thanks again 🙏!

@mymattcarroll
Copy link
Contributor Author

Thank you for your feedback! :)

I have been on holiday but still very interested in working on this. I will hopefully have time to look at this again at some stage this week.

@szdziedzic szdziedzic self-requested a review October 14, 2023 08:52
dependabot bot and others added 4 commits October 16, 2023 15:16
Bumps [undici](https://github.com/nodejs/undici) from 5.19.1 to 5.26.3.
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.19.1...v5.26.3)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
…undici-5.26.3

Revert "Bump undici from 5.19.1 to 5.26.3"
@mymattcarroll
Copy link
Contributor Author

@szdziedzic , do you have some time to have another look at this? The flag dependency has been removed.

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

Looks great! I tested it and I thing I noticed is that
https://github.com/oneblink/eas-cli/blob/main/packages/eas-cli/src/commands/project/init.ts#L240-L264
does not respect --non-interactive mode

I think we can change it to

 // if no owner field, ask the user which account they want to use to create/link the project
    let accountName = exp.owner;
    if (!accountName) {
      if (allAccounts.length === 1) {
        accountName = allAccounts[0].name;
      } else if (nonInteractive) {
        accountName = allAccounts[0].name;
        Log.log(`Using default account ${accountName} for non-interactive mode`);
      } else {
        const choices = ProjectInit.getAccountChoices(
          actor,
          accountNamesWhereUserHasSufficientPermissionsToCreateApp
        );

        accountName = (
          await promptAsync({
            type: 'select',
            name: 'account',
            message: 'Which account should own this project?',
            choices,
          })
        ).account.name;
      }
    }

    if (!accountName) {
      throw new Error('No account selected for project. Canceling.');
    }

@mymattcarroll
Copy link
Contributor Author

In regards to accountName no supporting non-interactive mode, I though it might be better to require the force flag to use the default account if a user has access to more than one. I'm happy to change to your suggestion if you believe the force flag requirement is overkill.

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

When it comes to accountName:

So if one uses nonInteractive without force this error will be thrown

if (!accountName) {
throw new Error('No account selected for project. Canceling.');
}
, right?

I believe that this is a confusing error message in such a case. I believe we should make it more actionable:

if (!accountName) { 
   if (nonInteractive) {
       throw new Error(`There are multiple accounts that you have access to: ${allAccounts.map(a => a.name).join(', ')}. Explicitly set the owner property in your app config or run this command with the --force flag to proceed with a default ${allAccounts[0].name} account.`)
   }
   throw new Error('No account selected for project. Canceling.'); 
 } 

@mymattcarroll
Copy link
Contributor Author

💯 agree! Ill update now to achieve the desired logic.

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

Amazing 🎉 LGTM!

Can you add a changelog entry for this PR by editing the CHANGELOG.md file, please 🙏?

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

one last tweak to make changelog linter happy

Co-authored-by: Szymon Dziedzic <sz.dziedz@gmail.com>
@szdziedzic szdziedzic merged commit 90a2612 into expo:main Sep 9, 2024
5 of 6 checks passed
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.

3 participants