Skip to content

cmds/core/gosh: replace go-prompt with bubbline#2606

Merged
MDr164 merged 5 commits into
u-root:mainfrom
MDr164:gosh-bubbline
Mar 27, 2023
Merged

cmds/core/gosh: replace go-prompt with bubbline#2606
MDr164 merged 5 commits into
u-root:mainfrom
MDr164:gosh-bubbline

Conversation

@MDr164
Copy link
Copy Markdown
Contributor

@MDr164 MDr164 commented Jan 30, 2023

As go-prompt used to cause issues this commit is replacing it. The bubbline library is actively maintained and more features complete than go-prompt was. This also addresses ctrl-c handling, a proper history and better autocomplete.

@MDr164 MDr164 force-pushed the gosh-bubbline branch 2 times, most recently from 229f006 to f306a5d Compare January 30, 2023 18:16
@MDr164
Copy link
Copy Markdown
Contributor Author

MDr164 commented Jan 30, 2023

TODO: Fix tests

@rminnich
Copy link
Copy Markdown
Member

we probably ought to get this done or close it.

@MDr164
Copy link
Copy Markdown
Contributor Author

MDr164 commented Mar 20, 2023

we probably ought to get this done or close it.

I'll be getting it in shape this week and fix the unit tests.

@MDr164 MDr164 marked this pull request as ready for review March 20, 2023 11:50
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2023

Codecov Report

Patch coverage: 39.82% and project coverage change: +3.05 🎉

Comparison is base (bcd2ee0) 69.07% compared to head (b4eec4b) 72.13%.

❗ Current head b4eec4b differs from pull request most recent head 82325a2. Consider uploading reports for the commit 82325a2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
+ Coverage   69.07%   72.13%   +3.05%     
==========================================
  Files         112      354     +242     
  Lines        6452    37386   +30934     
==========================================
+ Hits         4457    26970   +22513     
- Misses       1995    10416    +8421     
Impacted Files Coverage Δ
cmds/core/gosh/gosh.go 20.23% <18.46%> (-33.77%) ⬇️
cmds/core/gosh/completer.go 68.75% <68.75%> (-31.25%) ⬇️

... and 249 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MDr164
Copy link
Copy Markdown
Contributor Author

MDr164 commented Mar 20, 2023

we probably ought to get this done or close it.

Ready for review and testing

@MDr164 MDr164 added the Awaiting reviewer Waiting for a reviewer. label Mar 20, 2023
@MDr164 MDr164 linked an issue Mar 20, 2023 that may be closed by this pull request
@rminnich rminnich added Awaiting author Waiting for new changes or feedback for author. and removed Awaiting reviewer Waiting for a reviewer. labels Mar 20, 2023
Comment thread cmds/core/gosh/gosh.go Outdated
@MDr164 MDr164 added Awaiting reviewer Waiting for a reviewer. and removed Awaiting author Waiting for new changes or feedback for author. labels Mar 23, 2023
@rminnich rminnich added automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author. and removed Awaiting reviewer Waiting for a reviewer. labels Mar 27, 2023
MDr164 added 5 commits March 27, 2023 19:27
As go-prompt used to cause issues this commit is replacing it.
The bubbline library is actively maintained and more features
complete than go-prompt was. This also addresses ctrl-c handling,
a proper history and better autocomplete.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
@MDr164 MDr164 merged commit 3b2fee5 into u-root:main Mar 27, 2023
@MDr164 MDr164 deleted the gosh-bubbline branch March 27, 2023 18:12
@10000TB
Copy link
Copy Markdown
Member

10000TB commented Apr 26, 2023

Bubbleline is not a repo with a lot of public interests atm, and it carries a series dependencies on other repos

Did we agree it is a repo we can rely on ? -- asking to see if it is worth importing in for our internal use case.
It means we have to pull in all its dependent repos in order to use Bubbleline. Without reasonable number of
public adoptions, I would be concerned with the reliability, and it potentially increasing security attach surface

@10000TB
Copy link
Copy Markdown
Member

10000TB commented Apr 26, 2023

Appreciate you all's replies here soon -- need a decision soon as we need latest version of u-root to resolve a list of issues.

@rminnich @MDr164 @mvdan

did we consider submitting contributions for the "more features" into go-promopt ?

@10000TB
Copy link
Copy Markdown
Member

10000TB commented Apr 27, 2023

ping

@MDr164
Copy link
Copy Markdown
Contributor Author

MDr164 commented Apr 28, 2023

Well go-prompt is dead and has several issue that need fixing. I don't have the capacities to do so, so if you want to maintain go-prompt and fix all the open issues then we can revert that change. Bubbline is a fairly young project, that's why there is not so much "public interest" yet (also not sure how you measure that). Bubbline has none of the aforementioned issues and Raphael is still actively maintaining this repository. I'm not familiar with your internal guidelines but from a purely open-source perspective bubbline fixes more issues than it creates.

@10000TB
Copy link
Copy Markdown
Member

10000TB commented Apr 29, 2023

One of the the main issues raised here is that Bubbleline itself assembles features implementations from an array of other new GitHub repos.

Pulling in new uroot, means importing in Bubbleline into our software supply chain. That also mean we import / download the same array of GitHub repos bubbline depends on to be able to build bubbline.

Bubbline repo is not self sufficient (on standard golang libraries).

We are looking at 2+ weeks of SWE hours to set up all dependencies needed by bubbline.

I don't believe we need the new features, but we would need bear the setup cost that bubbline brings in.

@10000TB
Copy link
Copy Markdown
Member

10000TB commented May 2, 2023

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gosh should support simple scripts

4 participants