-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: env open @W-9104301@ #31
Conversation
src/commands/env/open.ts
Outdated
} else { | ||
const browser = flags.browser; | ||
this.log(`Opening ${nameOrAlias} in ${browser ? browser : 'the default browser'}.`); | ||
await open(url, { app: { name: flags.browser } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put some logic into plugin-login to use open
's cross-platform browsers: https://github.com/salesforcecli/plugin-login/blob/main/src/commands/login/org.ts#L78-L84
Just an idea though - not sure it's helpful enough to put here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I agree with https://github.com/salesforcecli/plugin-env/pull/31/files#r641762042
Heya @amphro found three issues while QA'ing. First, doesn't seem like you can open scratch orgs. I know you can't normally log into them unless you explicitly create a username/password though so not sure if this is expected or not, but either way we'd probably want the error message to be something different, not "Can't find the environment" right? Second when you don't specify an env via And third, you can see this in the picture above too, you can specify any random string for a browser and it pretends that it's actually using that, when in reality nothing opens up. |
I could open scratch orgs just fine @RodEsp - I fixed the other issues though. Can you give it another try? |
I changed it to an error, because we said that the defaults should be different for |
I'm happy to merge and open a GUS bug for the above if you want to just address it later. |
What does this PR do?
Adds an
env open
commandWhat issues does this PR fix or reference?
@W-9104301@