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

CLI: Fix open browser handling in interactive CLI #10461

Merged
merged 2 commits into from Jan 6, 2022
Merged

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Jan 6, 2022

When watching Steve's first steps demo (internal) we can notice that opening a browser window to set up a new provider doesn't work.
It appears bug on our side is that we pass ANSI colors decorated URL to open command, and that's not a valid URL. Unfortunately, this library doesn't report on that, but just silently runs it (with no effect). This patch fixes that.

Additionally upgraded the open browser logic, and at the same time unified it with the one we use in the dashboard plugin for logging in. There's no longer a need to keep hardcoded internally open implementation as we dropped support for Node.js v6 with v2 major.

/cc @mnapoli

It's to unify open browser method with dashboard plugin, and this method feels more solid
@medikoo medikoo added bug cat/cli cat/cli-onboarding Interactive CLI onboarding labels Jan 6, 2022
@medikoo medikoo self-assigned this Jan 6, 2022
@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #10461 (29dc1c9) into master (c67a3f1) will increase coverage by 0.42%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10461      +/-   ##
==========================================
+ Coverage   85.44%   85.87%   +0.42%     
==========================================
  Files         336      335       -1     
  Lines       14004    13921      -83     
==========================================
- Hits        11966    11954      -12     
+ Misses       2038     1967      -71     
Impacted Files Coverage Δ
lib/utils/openBrowser.js 64.70% <16.66%> (ø)
lib/cli/interactive-setup/aws-credentials.js 87.85% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c67a3f1...29dc1c9. Read the comment docs.

Copy link
Contributor

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Nice!

@medikoo medikoo requested a review from pgrzesik January 6, 2022 13:41
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Great catch, I thought that this issue with browser not opening was caused by the fact that Steve was using VM

@medikoo medikoo merged commit 7ebe133 into master Jan 6, 2022
@medikoo medikoo deleted the 0106-switch-to-open branch January 6, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cat/cli cat/cli-onboarding Interactive CLI onboarding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants