Skip to content

[CLI-2064] Only check for plugins in top-level directories in $PATH#1481

Merged
Brian Strauch (brianstrauch) merged 2 commits intomainfrom
CLI-2064
Oct 27, 2022
Merged

[CLI-2064] Only check for plugins in top-level directories in $PATH#1481
Brian Strauch (brianstrauch) merged 2 commits intomainfrom
CLI-2064

Conversation

@brianstrauch

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

We should not be recursing all the way through the user's $PATH directories. This is expensive and results in major slowdowns, especially on Windows systems.

References

CLI-2064

Test & Review

Updated all tests

@brianstrauch Brian Strauch (brianstrauch) requested a review from a team as a code owner October 25, 2022 23:55
Copy link

Choose a reason for hiding this comment

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

It may be useful to include a negative test case for plugin search to make sure we don't regress on this in the future, i.e. add a new test plugin to at test/fixtures/input/plugin/no-peeking/confluent-really-no-peeking and make sure the CLI doesn't find it. If you've already done this and I missed it, please ignore my comment and take a gold star ⭐.

I haven't taken a deep dive into the code, but I'm wondering why running confluent without any args would scan the PATH directories in the first place? I would assume that the only time a full, file-by-file scan of PATH directories needs to happen is when the user runs confluent plugin list or when the user enters an unrecognized argument that needs to be resolved to a plugin.

@brianstrauch
Copy link
Author

Brian Strauch (brianstrauch) commented Oct 26, 2022

I would assume that the only time a full, file-by-file scan of PATH directories needs to happen is when the user runs confluent plugin list or when the user enters an unrecognized argument that needs to be resolved to a plugin.

Alex Sweet (@asweet-confluent) The idea was:

  • A customer writes a plugin called confluent-xxx.sh.
  • We also write a new built-in command called confluent xxx.
  • The customer updates the CLI.
  • The customer should be alerted immediately that there's a naming conflict with one of their plugins.

I suppose we could wait to alert them when they actually run the overlapping command...

@asweet-confluent

I would assume that the only time a full, file-by-file scan of PATH directories needs to happen is when the user runs confluent plugin list or when the user enters an unrecognized argument that needs to be resolved to a plugin.

Alex Sweet (@asweet-confluent) The idea was:

* A customer writes a plugin called `confluent-xxx.sh`.

* We also write a new built-in command called `confluent xxx`.

* The customer updates the CLI.

* The customer should be alerted immediately that there's a naming conflict with one of their plugins.

I suppose we could wait to alert them when they actually run the overlapping command...

Makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

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

This should speed things up!

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.

3 participants