Skip to content

[CLI] Restore the "login" argument handler #1985

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

Merged
merged 2 commits into from
Nov 5, 2024
Merged

[CLI] Restore the "login" argument handler #1985

merged 2 commits into from
Nov 5, 2024

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Nov 5, 2024

This PR restores support for login in the CLI using a login argument.

The support was previously removed in #1811.

Testing Instructions (or ideally a Blueprint)

  • Run bun ./packages/playground/cli/src/cli.ts server --login
  • Open Playground and see that you are logged in

@bgrgicak bgrgicak self-assigned this Nov 5, 2024
@bgrgicak bgrgicak requested a review from a team November 5, 2024 15:49
@bgrgicak bgrgicak added the [Type] Bug An existing feature does not function as intended label Nov 5, 2024
@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Nov 5, 2024

@adamziel I realized my mistake with removing login support. While working on #1811 I kept running into issues with login and made an incorrect assumption that it won't work anymore with autologin.

What caused the confusion for me is that I expected the --login argument to override the Blueprint, but we don't override Blueprints in the CLI.

let blueprint: Blueprint | undefined;
if (args.blueprint) {
blueprint = args.blueprint as Blueprint;
} else {
blueprint = {
preferredVersions: {
php: args.php as SupportedPHPVersion,
wp: args.wp,
},
login: args.login,
};
}

Should we allow the wp, php, and login arguments to override Blueprints?
We did that recently on the Website with query arguments and Blueprints, so it should make the experience more consistent.

@@ -104,7 +104,7 @@ async function run() {
})
.option('debug', {
describe:
'Return PHP error log content if an error occurs while building the site.',
'Print PHP error log content if an error occurs during Playground boot.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related discussion #1983 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you! a --debug flag that doesn't cover the entire lifecycle of the app seems confusing to me. What do you think about either renameing it to --debug-boot or making it work after the boot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add request debugging. Here is an issue to track it #1988

@adamziel
Copy link
Collaborator

adamziel commented Nov 5, 2024

Should we allow the wp, php, and login arguments to override Blueprints?

Potentially – this is a nice moment to explore reusing the same code paths for resolving the Blueprint in the web app and in the CLI. Would doing that be useful? If yes, what would be the most effective way to do it?

@adamziel adamziel merged commit f991981 into trunk Nov 5, 2024
10 checks passed
@adamziel adamziel deleted the fix/cli-login branch November 5, 2024 20:04
@adamziel adamziel changed the title [CLI] Fix login argument [CLI] Restore the "login" argument handler Nov 6, 2024
@bgrgicak
Copy link
Collaborator Author

bgrgicak commented Nov 8, 2024

Potentially – this is a nice moment to explore reusing the same code paths for resolving the Blueprint in the web app and in the CLI. Would doing that be useful? If yes, what would be the most effective way to do it?

Thanks! I will leave it as is for now, but if it comes up in the future we can take a look at it.

@bgrgicak
Copy link
Collaborator Author

Thanks! I will leave it as is for now, but if it comes up in the future we can take a look at it.

We already have an open issue to allow the wp, php arguments to override Blueprints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package][@wp-playground] CLI [Type] Bug An existing feature does not function as intended
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants