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

Make script/debug a simple wrapper over the CLI #5733

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

deivid-rodriguez
Copy link
Contributor

Not sure if this makes sense to you, but today I wanted to use a full CLI run, with my development copy of dependabot-core code, but without a debugging shell. And the current script/debug script doesn't let you do that.

I thought about removing the hardcoded --debug flag, and if you want a debugger console, you can pass the --debug flag explicitly. Of course, that's more typing, so alternatively we could, for example:

  • Use dep for the script name.
  • Keep script/debug and do what I'm doing here in a separate script.

Does this make sense?

@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner September 15, 2022 19:36
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

👍 The only thing we lose with this is discoverability. And IMO it should be intuitive for someone to run this with --help and that should document --debug... if it's not, we should fix that (but separate to this PR)

I'm a fan of few scripts where possible, so prefer not to have separate run + debug script (esp for only one flag!)

My one question since I've never played with this script yet is where is this --debug flag even get passed to / kick in? Is there something in the dockerfile that is dropping the dependabot CLI go binary into this folder? I checked but didn't see anything... maybe I'm just blind though.

@jakecoffman
Copy link
Member

Makes sense 👍

@jakecoffman
Copy link
Member

@jeffwidman You have to install the CLI manually for now, it's not used anywhere beside the smoke tests where it gets downloaded on-demand each time.

@deivid-rodriguez
Copy link
Contributor Author

How about script/dep update bundler ...? Reads nice and short!

@jakecoffman
Copy link
Member

I'm up for anything! You can also add an alias in your shell to make it even shorter dep update bundler ... 😍

@jeffwidman
Copy link
Member

jeffwidman commented Sep 15, 2022

script/dep to me is ambiguous... if I parachuted into this folder as a new dev, I'd wonder is that short for "dependabot" or "dependency" or is it some other random Ubuntu tool that we needed to script or something?? Rather keep it a little longer and use tab-completion for speed, or if you're paranoid of carpal tunnel 😀 use a custom local alias like Jake suggested.

alternatively, given the names of the other scripts in this folder, could do script/run as that'd make it clear it's the "normal" entrypoint within this folder. I actually prefer that as script/dependabot is a little ambiguous of what it's doing (unless you happen to remember that the CLI is generally called dependabot)

Also, to be clear, these are just suggestions... not something I feel strongly about either way.

@deivid-rodriguez
Copy link
Contributor Author

Renamed to dep!

@deivid-rodriguez
Copy link
Contributor Author

script/dep to me is ambiguous... if I parachuted into this folder as a new dev, I'd wonder is that short for "dependabot" or "dependency" or is it some other random Ubuntu tool that we needed to script or something

I think both abbreviations work and read great 😅.

But I'll update it back since I don't think it matters too much!

@deivid-rodriguez
Copy link
Contributor Author

dependabot matches the name chosen for the CLI, so makes sense to stick to that!

If you want to use the interactive debug mode, you can still pass
`--debug`, but I think this makes it more useful!
@jeffwidman jeffwidman merged commit 1730ed2 into main Sep 15, 2022
@jeffwidman jeffwidman deleted the deivid-rodriguez/cli-wrapper branch September 15, 2022 23:13
@jeffwidman
Copy link
Member

I went ahead and merged since this one was green and up to date, that way it's one less PR that we have to play the "rebase" game on.

@pavera pavera mentioned this pull request Oct 31, 2022
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.

None yet

3 participants