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

feat: support targeting browser in login command #537

Merged
merged 2 commits into from Nov 30, 2022
Merged

feat: support targeting browser in login command #537

merged 2 commits into from Nov 30, 2022

Conversation

ghost
Copy link

@ghost ghost commented Nov 16, 2022

What does this PR do?

Add support for targeting browser in login command

What issues does this PR fix or reference?

forcedotcom/cli#1465

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @aemounib to sign the Salesforce.com Contributor License Agreement.

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

Hey @aemounib 👋🏼 , I left some minor suggestions but this looks great!

@ghost
Copy link
Author

ghost commented Nov 30, 2022

Thanks for the review, the suggestions have been implemented.

@RodEsp
Copy link
Contributor

RodEsp commented Nov 30, 2022

QA

✔️ Running auth:web:login --browser edge opens a windows on Edge
✔️ Running auth:web:login --browser chrome opens a windows on Chrome
✔️ Running `auth:web:login --browser firefox` opens a windows on Firefox

✔️ Running auth:web:login without the --browser flag uses my default browser

❌ If Firefox isn't installed and you run auth:web:login --browser firefox the command just hangs indefinitely. I assume the same happens for the other browsers. If possible, it should print an error stating the chosen browser isn't available and then exit. @cristiand391, do you think we should do this as a separate PR or fix it in this one?

@cristiand391
Copy link
Member

@RodEsp I tried running sfdx force:org:open -u <org> --browser edge (I don't have Microsoft Edge installed) and it continues successfully instead of failing, I looked at how open works and seems it depends on which OS you are running it will use different commands to open the browser.
https://github.com/sindresorhus/open/blob/main/index.js#L105

I'm ok with fixing this in a a separate PR since we will need to do it on plugin-org too and should work on all platforms. 👍🏼

@RodEsp RodEsp merged commit 6eb35e0 into salesforcecli:main Nov 30, 2022
@RodEsp
Copy link
Contributor

RodEsp commented Nov 30, 2022

Thanks for the contribution @aemounib!

@RodEsp
Copy link
Contributor

RodEsp commented Nov 30, 2022

Btw, I created this issue, forcedotcom/cli#1830, to track the failed QA and marked it with the help wanted tag.
And I started this PR, #551, for a fix but there is still an issue with the CLI not exiting. If you want to give it a shot @aemounib it would be most appreciated!

@ghost
Copy link
Author

ghost commented Dec 3, 2022

@RodEsp For sure. I will give it a shot, thanks.

@ghost ghost deleted the browser-flag branch December 3, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants