这是indexloc提供的服务,不要输入任何密码
Skip to content

feat(cobra): add bin command #906

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 45 commits into from
May 7, 2022
Merged

feat(cobra): add bin command #906

merged 45 commits into from
May 7, 2022

Conversation

samchouse
Copy link
Contributor

@samchouse samchouse commented Mar 19, 2022

What this PR brings:

@vercel
Copy link

vercel bot commented Mar 19, 2022

@xenfo is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 21, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/DdUjj21pFd1QEXjR1M35iF6pejGq
✅ Preview: https://turbo-site-git-fork-xenfo-feat-cobra-bin.vercel.sh

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

As a general note, it looks like there's a lot going on here. Before we dive into specific code changes, can you describe some of the changes like:

  • why we want the logger package here, vs the one we're currently using
  • the intended use for some of the cmdutil functions, like createTraceFile, createHeapFile, etc. as they don't seem to be called in this PR
  • what's the motivation for the changes to printf.go?

It looks like there's also some churn in naming and comments. I'm not necessarily opposed to renaming things, but this might not be the PR to do it. For instance, TurboVersion -> Version or Config.IsLoggedIn -> Config.IsAuthenticated.

I would expect this PR to be primarily focused around the changes to bin.go, and have the existing BinCommand delegate to the cobra-based BinCmd. Once we iterate our way through the various commands making this change, we can do one or more PRs to tie it all together in main.go.

@samchouse
Copy link
Contributor Author

This PR is for setting up for more Cobra commands and in the final PR I would link everything up into the root Cobra command.

@samchouse samchouse changed the title feat(cmd): cobra bin feat(cobra): setup for cobra and add bin command Mar 22, 2022
@samchouse samchouse requested a review from gsoltis March 22, 2022 11:05
@samchouse samchouse mentioned this pull request Mar 22, 2022
7 tasks
@gsoltis
Copy link
Contributor

gsoltis commented Mar 22, 2022

I understand that these are setting up for later integration, but if we merge this PR, they are unused code. I would prefer that these utilities get pulled in when they are used, even if just by a single command, rather than in advance.

Similarly with the actual bin command here. It's defined, let's actually wire it up so it gets used and isn't dead code.

@samchouse
Copy link
Contributor Author

samchouse commented Mar 22, 2022

My thinking was to use this PR to setup for others, after this one I can create all the PRs for the rest of the commands at the same time and everything I need for them is already there. This would be done as soon as this is merged, everything is ready. I also don't see why we would want to mix in Cobra commands with current one as it would require moving code out of the command and making a deps struct of sorts that would be deleted on final linking. If you want me to do this I will though.

@gsoltis
Copy link
Contributor

gsoltis commented Mar 22, 2022

I would look at login as a model here. My goal with requesting it be done this way is to keep main deployable with ideally no dead code (I am aware there is some existing cruft, but lets not add to it). This also lets us exercise the new code as we go, without having to wait for the full, large change to land.

@samchouse
Copy link
Contributor Author

I think it's best to keep all utility files in this PR so that we can merge other PRs faster. Eg: xyz might not be used in bin but it would be used in login and link, by merging it in this PR it would allow login and link to be merged faster without waiting on the other to be merged.

@gsoltis
Copy link
Contributor

gsoltis commented Mar 22, 2022

It is unfortunately not really possible to review unused code being added, since part of what the review includes is "do we need this code". For utilities that you would like to merge first, especially if they would be useful for login, I would suggest making a separate PR integrating them into the existing login command. That will be a smaller and easier-to-review PR, and also make the utilities available for subsequent commands. That would also be a good way to make a case for these added utilities vs what we already have, since we can see them in context.

There's a lot of surface area to this feature. It encompasses nearly the entire UX of the CLI. So I apologize if merging ends up going a little slower, but I think the priority has to be stability vs the overall speed of merging the PRs.

@samchouse samchouse changed the title feat(cobra): setup for cobra and add bin command feat(cobra): add bin command Mar 22, 2022
@samchouse
Copy link
Contributor Author

Alright, I've reduced this PR to only the bin command.

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this. I took a first pass at reviewing.

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

This change currently affects 29 files. I think for the task of moving bin to cobra, we ideally want to just modify bin.go. would it be possible to remove the unrelated changes so we can focus on the cobra usage?

@samchouse
Copy link
Contributor Author

samchouse commented Mar 23, 2022

I'll create another PR for formatting then.

Edit: #927

@samchouse
Copy link
Contributor Author

If we're going to be moving to cobra then we ideally shouldn't be depending on another CLI framework. cobra is also built differently than the current framework, we can't pass numbers up the stack anymore, we have to pass errors. This makes it easier to have more code duplication, etc.

For example with the new UI we can simply do this to pass an error up. It's one line, it's always in sync and has minimal code duplication.

// ch.Ui is the new UI in this example

// for exit code 1
return ch.Ui.Errorf("${UNDERLINE}something went wrong${RESET}")

// OR

// for any other exit code
return &cmdutil.Error{
  ExitCode: 123,
  Err: ch.Ui.Errorf("${UNDERLINE}something went wrong${RESET}"),
}

It's counter part would be this, which has a lot of fluff and has more chances for getting out of sync or some other errors happen.

// ch.Ui is the old UI in this example

msg := util.Sprintf("${UNDERLINE}something went wrong${RESET}")
ch.Ui.Error(msg)
return fmt.Errorf(msg)

// OR

msg := util.Sprintf("${UNDERLINE}something went wrong${RESET}")
ch.Ui.Error(msg)
return &cmdutil.Error{
  ExitCode: 123,
  Err: msg,
}

Furthermore, all the commands are already written with new new UI, and changing back to the old one would require me to rewrite the commands again with the old UI. Then if we do want to use the new UI, we would have to rewrite the commands once more. That would be 3 total rewrites vs 1 total rewrite.

I understand your trying to keep this PR simple and just the bare minimum for the bin command to work, but I really do believe the new UI is essential in the move to cobra. Overall, it's just cleaner and safer to use the new UI package and nothing should break unless go introduces bugs into fmt.

@gsoltis
Copy link
Contributor

gsoltis commented Apr 25, 2022

I understand it may be frustrating, but I cannot approve starting a UI migration at this point. We have several internal migrations in progress right now, including to cobra, all of which have been a source of bugs or toil. It's possible that once we complete some of them, it may make sense to look at our UI usage as well. In the meantime, I am not aware of any issues with depending simultaneously on cobra and cli.Ui, and our cli.Ui usage so far has been largely free of problems.

All of the migrations, including this one, are being done via scoped, small PRs. This has a few advantages: easier to review and spot bugs, and in the event that a bug is not caught, a smaller blast radius in terms of what needs to be reverted.

I would be happy to review PRs switching individual commands over to cobra. Ideally these would do as little as possible to the actual functioning of the commands, and be focused solely on ensuring we have the right commandline arguments, help text, etc. Bonus points for improving the testability of the commands, but in many cases there are other changes that need to happen first.

@samchouse samchouse requested a review from gsoltis April 26, 2022 01:35
@samchouse samchouse requested a review from gsoltis April 28, 2022 22:39
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

This is coming along!

samchouse and others added 2 commits May 4, 2022 06:42
Co-authored-by: Greg Soltis <gsoltis@gmail.com>
@samchouse samchouse requested a review from gsoltis May 4, 2022 10:49
@vercel
Copy link

vercel bot commented May 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
turbo-site ✅ Ready (Inspect) Visit Preview May 5, 2022 at 10:48PM (UTC)

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

Awesome, it looks like there's a minor merge conflict in main.go, then let's get this merged!

@gsoltis
Copy link
Contributor

gsoltis commented May 5, 2022

@xenfo can you also run goimports on main.go? It looks like it's currently failing a lint check.

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

Let's get this merged! Thanks for sticking with it!

@kodiakhq kodiakhq bot merged commit ba2affb into vercel:main May 7, 2022
@samchouse samchouse deleted the feat/cobra-bin branch May 7, 2022 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants